Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1712;
Closes #1713 / Closes #1714
Overview
Mapping code changes to original issues:
isel()with xr.DataArray indexer along grid dimension fails to include indexer's coordinates in result #1712 was to define_assign_grid_dim_indexer_coords_if_appropriatein coords.py, and utilize it during UxDataArray.isel() and UxDataset.isel(). Also updated isel() and sel() docstrings accordingly.UxDataset.isel()silently fails to slice when providing grid dim not in the dataset #1713 was incredibly simple, just needed to check if any grid dim is in da.dims when looping through data arrays in the dataset, inUxDataset._slice_dataset_from_grid. While touching this method though, I decided to clean it up further than this (see relevant "expansion of scope" details below).UxDataset._slice_dataset_from_grid. I fully deleted those lines instead of "trying to fix them" because I don't think they made sense to have in the first place. See point (2) below for more details.In all cases, also updated test_indexing.py accordingly. For 1713 and 1714, it was sufficient to just uncomment some existing lines which now serve as regression tests for these issues. For 1712, it also included building new tests inside
test_indexing_by_dataarray(), which led to discovering #1758.Expansion of scope: updated
UxDataset._slice_dataset_from_gridin a few different ways to increase consistency with UxDataArray indexing methods:UxDataset._slice_from_grid(to match naming conventions ofUxDataArray._slice_from_grid)UxDataset.isel()#1352), and they were not being covered by any example within the test suite (removing them does not cause any existing tests to fail).self.data_varswhengrid_dim in da.dims and not hasattr(da, "_slice_from_grid"). This branch would never be reached. (Right now, all data_vars are UxDataArrays. Eventually, maybe only data_vars with a grid dimension will be UxDataArrays. Either way, there should never be a data_var which both has a grid dim and is an xr.DataArray.)grid_dimorgrid_indexerargs; these were only being used in the now-removed parts of the code from points (2) and (3) above.UxDataArray._slice_from_gridreturns a UxDataArray, not an xr.DataArray.)Tiny expansions of scope:
Updated UxDataArray._slice_from_grid()to return an object of type(self), instead of UxDataArray. This has no extra cost, and makes uxarray slightly more object-oriented friendly. Before this change, on subclasses likeclass MyCoolArray(UxDataArray), isel() would return a UxDataArray instance if indexing by exactly 1 grid dim (e.g. isel(n_face=7)), but an instance of the subclass (MyCoolArray) if indexing by 0 grid dims (e.g. isel(time=0)) or if additionally indexing by something other than a grid dim (e.g. isel(time=0, n_face=7)). After this change it will always return an instance of the subclass (MyCoolArray) regardless of which dimensions get indexed.UxDataArray.isel()code (i.e. calling the resultresultinstead ofdaorxarr, and updating the value ofresultinside theif indexers:...block instead of returning from directly inside that block) to emphasize that the implementation is nearly identical toUxDataset.isel(). Maybe some day the full implementation, or at least a majority of it, could be moved to a shared parent class (something likeUxDataContainer?), as a way to help avoid any more unexpected behavior differences between UxDataArray.isel() and UxDataset.isel().Tiny expansions of scope unrelated to original issue:
import uxarrayfrom top of dataset.py and dataarray.py files. Pretty sure it doesn't belong there; importing a full package from within itself is not standard practice. Removing this made it necessary to make a few other tiny updates:type(self)instead ofuxarray.UxDataset, and to useUxDataArrayinstead ofuxarray.UxDataArray.type(self)instead ofuxarray.UxDataArray.from uxarray.core.dataset import UxDatasetinstead of usinguxarray.core.dataset.UxDataset.PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: GitHub Copilot's inline code suggestions, plus a small discussion with Claude for debugging/understanding syntax of super().