Skip to content

Add insert/remove_axis_inplace for IxDyn arrays #533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

jturner314
Copy link
Member

Without these methods, users must have ownership of dynamic-dimensional arrays to insert/remove axes, which is unnecessarily restrictive.

This is motivated by #528.

Without these methods, users must have ownership of
dynamic-dimensional arrays to insert/remove axes, which is
unnecessarily restrictive.
bluss
bluss previously approved these changes Nov 11, 2018
Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@bluss bluss dismissed their stale review November 11, 2018 20:56

Thinking more

let len = self.len_of(axis);
assert_ne!(len, 0, "Length of removed axis must be nonzero.");
self.dim = self.dim.remove_axis(axis);
self.strides = self.strides.remove_axis(axis);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess removing an axis can never make us lose the first element (at index [0, 0, ..]) right, so we don't need to recompute the array's pointer here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. We need to assert that the axis length is nonzero for a similar reason.

@bluss
Copy link
Member

bluss commented Nov 11, 2018

Remove is surprising, maybe just make it clear to the user that it's like an in place slice, we hide some elements and can't get them back. Weird for owned arrays, but we already have the same thing in slicing, so it makes sense.

@jturner314
Copy link
Member Author

Remove is surprising, maybe just make it clear to the user that it's like an in place slice, we hide some elements and can't get them back.

I can certainly document this, but I agree it's still weird.

I've always thought that .remove_axis() on ArrayBase was a pretty weird method because the name doesn't clearly describe what it's doing. The fact that it panics when the axis has length 0 also makes me uncomfortable. .remove_axis(Axis(x)) is equivalent to .into_subview(Axis(x), 0) anyway, so I'm actually in favor of removing .remove_axis() entirely.

The question remains, though, what to do with .remove_axis_inplace() for IxDyn arrays. One idea is the following:

  • Rename the existing .subview_inplace() method on ArrayBase to .narrow_axis() or something.
  • Add a .subview_inplace() method for IxDyn arrays that actually removes the axis. (Don't add a .remove_axis_inplace() method.)

The problem with this is that changing .subview_inplace() like this is a breaking change in behavior that would cause problems that wouldn't be caught by the compiler, since both behaviors (keeping the axis or removing it) have the same types for IxDyn arrays.

A workaround for the difficult-to-catch breaking change issue is the following. I prefer this naming scheme anyway because it's more similar to the slice/slice_axis methods:

  • Rename .subview() to .index_axis()
  • Rename .subview_mut() to .index_axis_mut()
  • Rename .subview_inplace() to .narrow_axis()/.select_one()/.select_single()/.select_index()/etc.
  • Rename .into_subview() to .index_axis_move()
  • Add .index_axis_inplace() to ArrayBase<S, IxDyn>

What do you think?

@bluss
Copy link
Member

bluss commented Nov 11, 2018

Sounds good! I guess index_axis makes more sense too (the major connection we have is to the axis iters that produce the same views).

@jturner314
Copy link
Member Author

See #537, which does the renames mentioned above, except using .collapse_axis() instead of .narrow_axis() and additionally renaming .slice_inplace() to .slice_collapse().

@jturner314
Copy link
Member Author

Closing in favor of #537.

@jturner314 jturner314 closed this Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants