Skip to content

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

2 participants