Repository navigation
Conversation
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Thank you for putting this together, @sunxiuguo! This looks great. I read through all the code and the changes look good by-eye, and I also confirmed that the new tests are proper regression tests (at least some of them fail on main).
Interestingly, running the tests on main also hits another error which wasn't discovered in the initial issue report, specifically a confusing UnboundLocalError: cannot access local variable 'dim_ordered'... would occur whenever the dataset didn't include the correct lon_name and lat_name dims. This error has been fixed in this PR, which now raises a much clearer DimensionError in that case, instead.
Happy to approve this!
I do also have one tiny, non-blocking/stylistic comment: could you update the from_xarray docstring's explanation of ugrid_dims to include a brief example, something like For example, {"n_face": ("x", "y")}? The existing docstring doesn't really clarify what the dictionary mapping should be, but a tiny example like that makes it clear that the grid dimension is the key and the dataset dimensions belong in a tuple as the values.
erogluorhan
left a comment
erogluorhan
left a comment
There was a problem hiding this comment.
Overall I like this fix, thanks very much! One single inline comment below though
| dim_ordered = [lon_name, lat_name] | ||
|
|
||
| ds = ds.stack(n_face=dim_ordered) | ||
| if lon_name not in ds.dims or lat_name not in ds.dims: |
There was a problem hiding this comment.
I see where you're coming from with this check, but I think ds.dims might not be the same as source dims but can still align with the grid (probably in rare use cases though)
Cover renamed axes with rectangular and square grids and transposed storage. Clarify that missing dimensions refer to the selected mapping, without changing the guard or stacking behavior.
Internal discussions have clarified that from_xarray is not intended to support structured data; I need to re-review and make different recommendations, with this in mind.
dylannelson
left a comment
dylannelson
left a comment
There was a problem hiding this comment.
Minor change on the same line @erogluorhan made a comment about. Hope this resolves both concerns.
| dim_ordered = [lon_name, lat_name] | ||
|
|
||
| ds = ds.stack(n_face=dim_ordered) | ||
| if lon_name not in ds.dims or lat_name not in ds.dims: |
There was a problem hiding this comment.
| if lon_name not in ds.dims or lat_name not in ds.dims: | |
| if lon_name not in ds.dims and lat_name not in ds.dims and "n_face" in ds.dims: | |
| # Already flattened onto the Grid's faces (as open_dataset accepts). | |
| return ds | |
| if lon_name not in ds.dims or lat_name not in ds.dims: |
If the data is already on n_face, from_xarray raises DimensionError, but ux.open_dataset(grid, ds) on main accepts the same data. This lets that case through and still raises for everything else. This early return makes the two agree. The new tests have been checked and still pass.
Preserve canonical n_face inputs in from_xarray while retaining the missing-source-axis guard for partially structured data. Add eager, lazy, time, explicit-mapping and partial-axis regressions.
|
Just providing a brief reportback/update here for now: it's now unclear to me how much of this PR's changes will be desirable or necessary, in light of recent developments/clarifications in #1887. The main thing that caused me to originally agree with your proposed path forwards here was the way in which #1410 was solved, especially the corresponding regression test which explicitly used from_xarray() with structured data inputs, as we discussed in thread on your issue report (see e.g. #1828 (comment)). However, in #1887 it was recently clarified that from_xarray() is indeed not actually intended to support structured data inputs; the currently-proposed changes there would fully remove that regression test in favor of a small check that there is indeed a crash when calling from_xarray() with any structured data inputs. Because of all of this, I'm not sure how we'll end up proceeding here. I think it makes the most sense to delay further updates here until #1887 gets merged, and then reassess. Apologies for accidentally giving you the run-around on this one, @sunxiuguo! I'll plan to report back here with more definitive thoughts / suggested next steps, after that PR merges. |
Closes #1828
Overview
Use the Structured Grid's latitude-major, longitude-minor face order when mapping data onto
n_face, instead of inferring the order from the last data variable. This follows the agreed order clarification and preserves the existing(longitude, latitude)source-dimension metadata contract.(lat_name, lon_name), independently of variable insertion order or spatial axis order.DimensionErrorwhen the recorded spatial dimensions are absent, rather than silently returning unflattened data or failing with an incidental unbound variable.UxDataset.from_xarraydocstring.Only three files change. This is separate from #1827's Grid-reuse opening APIs. No DataArray-specific source or docstring changes are included; the shared-helper change incidentally removes its structured
data_varsAttributeError in an independent probe, but does not claim to resolve its entire API/documentation issue. Pre-existing coordinate-name/dimension-name metadata mismatch and descending-longitude sorting are not fixed here.Verification
Tested on main
65e9c4a15c620865e5412c96dd8fbe2052ac788c, Python 3.12.14, NumPy 2.5.4, Xarray 2026.9.0 and Dask 2026.8.0, using synthetic data in an isolated Linux environment.Not a full repository suite, multi-platform CI or ASV result. Upstream CI and benchmark approval remain with maintainers.
PR Checklist
General
Testing & Benchmarking
Documentation and Examples
AI Disclosure
AI Usage: OpenAI Codex for analysis, implementation, test development and independent review.