Skip to content
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

Fix xaxis_*_iterator shape and strides types #2747

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Stef-Sijben
Copy link

Checklist

  • The title and commit message(s) are descriptive.
  • Small commits made to fix your PR have been squashed to avoid history pollution.
  • Tests have been added for new features or bug fixes.
  • API of new functions and classes are documented.

Description

Fixes #2116.

As described by @dchansen in #2723:

This issue for both cases is that the strides_type and shape_type in axis_slice_iterator and axis_iterator are taken from the given xexpression, which is not correct for xtensor as the resulting strided_view should anyhow be dynamically sized.

The alternative would be to assign a dynamic strides and shape_type, but it was not clear to me what the correct replacement type should be.

So I changed the type of the shape and strides members in the returned views/iterators as follows:

  • For xaxis_iterator: std::vector<T>, where T is the value type of the input expression's shape or strides type.
  • For xaxis_slice_iterator: std::array<T, 1>, since the resulting view is always one-dimensional. Note that this is different from the currently returned shape and strides type in case of xarray input, so this breaks API/ABI for those cases. If that is not desired, this case should also use std::vector<T>.

For both iterators, this loses some compile-time information in certain cases. E.g.:

  • For xtensor_fixed inputs, the resulting view could also have compile-time shape and strides, but some more metaprogramming magic would be required to correctly detect that situation and select the correct types.
  • For xtensor inputs, xaxis_iterator's view could have fixed dimension of N-1.

Someone else would have to look into that if that's desired. However, I think the above paragraph would be more of an optimization, and this fix at least makes the iterators on xtensor and xtensor_fixed functionally correct, in the sense that their views have the correct shape and contain the correct elements.

I also made the iterator test cases type parameterized, so the existing test cases check all of xarray, xtensor, and xtensor_fixed as input types.

Test not only `xarray` inputs, but also `xtensor` and `xtensor_fixed`.
These are currently failing.

Also test some more cases with `column_major` inputs.
An `xaxis_slice_iterator` always refers to a 1d view, so just use an
array of size 1 for the shape and stride.
Another type would probably be more optimal in case of compile-time
fixed size (e.g. `xtensor_fixed`), but at least this is correct.

Always use runtime dimensionality `xaxis_iterator` shape and strides.
Other types would probably be more optimal in case of compile-time
fixed dimension and/or size (e.g. `xtensor`, `xtensor_fixed`), but at
least this is correct.
@hsanzg
Copy link

hsanzg commented Oct 11, 2024

Also fixes #2543.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible to use 1D and (N-1)D slice iteration with xt::xtensor?
2 participants