Repository navigation
Conversation
and pass through **kw_apply_ufunc
and other improvements (will put more detailed description in PR overview).
Sevans711
added
new feature
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
|
Re: ASV benchmarks, I think it looks okay. I am not expecting any performance changes, and I'm seeing no changes for the most part. The only exceptions are a small (~20%) slowdown in the 480km gradient benchmark (but no significant slowdown in the 120km gradient benchmark, so I think this might just be noise?), and a small increase to track_peakmem_gradient (~10%) but that also seems to occur randomly on different PRs which aren't touching gradient at all, so I think it's okay to ignore here? Requesting reviews because things mostly look okay. The readthedocs build isn't building right now but it's not this PR's fault, readthedocs just had a random issue. If reviewers can review the non-readthedocs parts for now (i.e. everything except the vector_calculus.ipynb changes) and then check the readthedocs parts in a later review, that would be awesome! |
cmdupuis3
left a comment
cmdupuis3
left a comment
There was a problem hiding this comment.
I have two speed enhancements on the cmd/2d+calculus branch.
Other issues:
- Results now inherit the input's attrs via apply_ufunc/binary ops (keep_attrs on in current xarray), so gradient/curl/divergence outputs carry the input's long_name/standard_name, which mislabels the derivative.
- Unit issues:
divergence(units=...)is now ignored. Also, curl and divergence units are missing their parentheses e.g. m/s/m vs. (m/s)/m
also adds tests for what the "units" attr should contain.
See discussion in #1573 for details. Basically, DeprecationWarning is not visible to users by default, but FutureWarning is.
|
The readthedocs build is now ready for review too! The animation at the end misses the last frame but that's a minor issue and it has been fixed upstream (see holoviz/holoviews#7113, holoviz/panel#8860); I think it's okay to keep as-is for now and allow it to resolve itself when the next panel release comes out (it looks like panel releases have been occurring every couple of months or so, and the last one was in August). Regarding @cmdupuis3's point (1): I can't reproduce the inputs' Regarding point (2): the divergence(units=...) being ignored is intentional (see point (4) in original post), but the fact that it was being silently ignored is not intentional. Swapped DeprecationWarning to FutureWarning because DeprecationWarnings are often hidden by default (as discussed in #1573). The missing parentheses (e.g. "(m/s)/m" not "m/s/m") was due to an inconsistency between curl/divergence units (which always put parentheses on everything, e.g. "(K)/m") and gradient units (which never put parentheses on anything, e.g. "K/s/m"). I've updated this so that parentheses are added only when there is a "/" in the input units, e.g. "m/s" --> "(m/s)/m", while "K" --> "K/m". Updated the Regarding the speedups on the I believe all points have been addressed and this is now ready for re-review! |
|
Nice feature — one helper routing the grid dim through Before I dig in, please close out Chris's remaining points. On the |
@rajeeja thank you, and, done! I still don't personally understand your reasoning for the sign flip; the relevant tests are completely insensitive to the actual values of the data. Nothing in the tests in question (the |
cmdupuis3
left a comment
cmdupuis3
left a comment
There was a problem hiding this comment.
Re (1):
Metadata like data_source or simulation_resolution is still true of a derivative, so that's okay, but standard_name, long_name, valid_min/valid_max/valid_range and actual_range describe the input field, and copied onto a derivative, they would be wrong. gradient() never sets long_name or standard_name, so the gradient of T comes out labelled air_temperature / Temperature. The valid_* attributes cause data loss. CF-aware readers (netCDF4-python by default, ncview, Panoply) mask values outside the valid range. A gradient of about 1e-6 K/m is far below valid_min=150, so the values masked when the file is read back.
There are also no deliberate rules deciding which attributes survive; the result falls out of xarray’s merge. With keep_attrs=True, binary operations keep the attributes that don’t conflict between operands. In curl(U, V) the two standard_names conflict and are dropped. The latitude-correction term, built from U, then adds eastward_wind back. In divergence(U, V) that term is built from V, so the divergence ends up labelled northward_wind. Neither is a correct name for a curl or a divergence.
import netCDF4
import numpy as np
import uxarray as ux
ds = ux.tutorial.open_dataset("outCSne30-vortex")
psi = ds["psi"]
psi.uxgrid.sphere_radius = 6.371e6
u = psi.rename("U").assign_attrs(
units="m/s", standard_name="eastward_wind", long_name="Zonal wind",
data_source="vortex test case",
)
v = psi.rename("V").assign_attrs(
units="m/s", standard_name="northward_wind", long_name="Meridional wind",
data_source="vortex test case",
)
T = psi.rename("T").assign_attrs(
units="K", standard_name="air_temperature", long_name="Temperature",
valid_min=150.0, valid_max=350.0,
)
# 1. gradient() never sets long_name/standard_name, so it keeps T's:
print(T.gradient()["zonal_gradient"].attrs)
# 2. divergence(U, V) ends up labelled with V's standard_name; curl(U, V) with U's
print(u.divergence(v).attrs["standard_name"]) # northward_wind
print(u.curl(v).attrs["standard_name"]) # eastward_wind
# 3. T's valid range is inherited by its gradient, so a CF-aware reader
# treats every gradient value (~1e-6 K/m) as out of range:
T.gradient()["zonal_gradient"].to_netcdf("dTdx.nc")
with netCDF4.Dataset("dTdx.nc") as nc:
x = nc["zonal_gradient"][:]
print(f"{np.ma.count_masked(x)} of {x.size} values masked on read")also fix missing gradient long_name and description, and set gradient=True in gradient output attrs, similarly to curl and divergence.
|
Re (1), sure, I agree those are confusing cases. I now see what you mean about long_name; it was being handled properly by curl and divergence, but not gradient (this is now fixed; I've added _VALUE_DESCRIBING_ATTRS = [
# What the quantity is
"standard_name", "long_name", "units", "units_metadata",
# Valid/observed values (may cause masking on netcdf read-back)
"valid_min", "valid_max", "valid_range", "actual_range",
# Missing-data sentinels
"_FillValue", "missing_value",
# Packing / scaling
"scale_factor", "add_offset", "_Unsigned",
# Categorical flags
"flag_values", "flag_masks", "flag_meanings",
# Provenance tied to the input's values
"cell_methods", "ancillary_variables", "standard_error_multiplier",
# might also consider "description" and "comment",
# but not including those here yet.
]Kept a leading underscore on this list to emphasize it might be changed at any time, it's an internal implementation detail; hoping this can avoid blocking this PR on a detailed discussion of exactly which names belong in this list, since it will be very easy to change later if needed. I also added a helper function to drop value describing attrs, and used it in appropriate places. Tested in |
|
As a sidenote, netcdf writers seem to complain when attrs' values are booleans, so I'm not sure if {"divergence": True}, {"curl": True}, {"divergence": True} is a great convention for calculus operators' results' attrs... but that seems to be the convention on |
|
@Sevans711 Yeah, there could definitely be more issues there. CF-compliant attributes are a giant cluster, so even what you see here may be deceptively oversimplified.
|
netcdf doesn't like boolean-valued attrs.
To confirm, we're not trying to solve all of them as part of this PR, though, right? This feels like another PR's worth of work, or possibly multiple PRs if the goal is to make it apply across all operations which alter results' values throughout all of uxarray.
Fixed by updating naming scheme to
Fixed by adding a
Fixed, great catch, I previously suspected deep=False was the default, but that is wrong, the default is deep=True! |
3 participants
Closes #1723
Overview
Adds support for applying calculus operators to 2D+ arrays (more dims than just the grid dimension), by creating the new
_apply_1dfunc_with_grid_core_dim()helper method and using it inside_compute_gradient(). The operators which now support 2D+ arrays are gradient(), curl(), divergence(), and scalardotgradient().Tests note: the tests which prove that these changes are compatible with dask arrays remain commented out for now; they should be uncommented when #1798 merges. See #1823 (comment) for more details.
While working on this I made numerous miscellaneous improvements related to calculus methods, expanding the scope of this PR as follows.
Small expansions of scope (examples):
xr.set_options(display_expand_data=False)). The corresponding icon still shows/hides, the default state is just "hidden" instead of shown.~with\~in one markdown cell (starts with "Pure rotation is incompressible...") which was rendering locally with strikethrough for some text instead of tildes.Sidenote about the notebook: there are a few cells in the middle which I didn't touch but the diff shows they were edited; this was done by ruff formatting.
Small expansions of scope (functionality):
Tiny expansions of scope (functionality):
grad_u_zonalandgrad_v_meridionalinstead of the confusingu = grad_u_zonalandv = grad_v_meridional.Expected Usage
Exactly the same syntax as the operators have currently, but they can now be applied to arrays with more dimensions:
PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: Claude for discussions and some code suggestions; GitHub Copilot's inline code suggestions