Fix indexing bugs in CoordinateTransformIndex
#10980
Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
bot
added
topic-indexing
topic-testing
topic-hypothesis
The return statement was incorrectly indented inside the for loop, causing only the first coordinate to be assigned to the DataArray. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
* main: Document limitations of cftime arithmetic (pydata#10653) Remove RTD htmlzip output format (pydata#10977) Support decoding unsigned integers to timedelta (pydata#10972) Use `._data` in `Variable._replace` (pydata#10969) small changes to the pixi env definitions (pydata#10976) Add GitHub Codespaces config for Pixi (pydata#10929) Fix workflow name to embed `matrix.pytest-addopts` (pydata#10970)
keewis
left a comment
keewis
left a comment
There was a problem hiding this comment.
I think there's a logic error in your tests, so it might be good to resolve that before merging
| def forward(self, dim_positions: dict[str, Any]) -> dict[Hashable, Any]: | ||
| return {name: dim_positions[name] for name in self.coord_names} | ||
|
|
||
| def reverse(self, coord_labels: dict[Hashable, Any]) -> dict[str, Any]: | ||
| return {dim: coord_labels[dim] for dim in self.dims} |
There was a problem hiding this comment.
not sure if I'm misunderstanding something here, but there appears to be something wrong: why do we use coordinate names to index dim_positions (which I think map dimension names to positions), and dimension names to index coord_labels?
(the example at https://xarray-indexes.readthedocs.io/blocks/transform.html#example-astronomy appears to support my mental model of the coordinate transform)
| for dim, size in sizes.items(): | ||
| transform = IdentityTransform([dim], {dim: size}, dtype=np.dtype(np.int64)) | ||
| index = CoordinateTransformIndex(transform) | ||
| ds = ds.assign_coords(xr.Coordinates.from_xindex(index)) |
There was a problem hiding this comment.
not sure if this will make this more readable (it does remove the emulated in-place assignment, at least), but it is possible to collect the coordinate objects and then use functools.reduce(operator.or_, coordinates) to merge them together.
| microseconds_offset = draw(st.integers(0, timespan_microseconds)) | ||
|
|
||
| return min_value + datetime.timedelta(microseconds=microseconds_offset) | ||
|
|
There was a problem hiding this comment.
these are neat, I remember looking for strategies like this before
| ---------- | ||
| draw : callable | ||
| The Hypothesis draw function (automatically provided by @st.composite). | ||
| sizes : dict[Hashable, int] |
There was a problem hiding this comment.
not sure how important this is, but napoleon will not parse these correctly, so we won't get links. This would work, though:
| sizes : dict[Hashable, int] | |
| sizes : mapping of hashable to int |
and the same for the instances below
3 participants
Fixes some severe bugs noticed downstream in rasterix
Disclaimer: had lots of help from Claude in writing the strategies but I have reviewed them closely