Conversation
The spherical metric term added in #1663 calls `other.values`, which materializes the field and forces the whole divergence to compute eagerly. Its companion in `curl` uses `.data` and stays lazy, so switch divergence to match.
The curl and divergence background cells give the planar formulas, and eight code cells hand-roll the operators out of gradient() components instead of calling curl()/divergence(). Since #1663 the library adds the spherical metric terms, so every worked example demonstrated something the library no longer computes. Add the metric terms to the math, switch the examples to the public API, and rewrite the constant-field discussion: curl and divergence of a constant field are u*tan(lat)/a and -v*tan(lat)/a, not zero. Both are now checked against their closed forms. Residual magnitudes in the prose are corrected to match what the notebook actually prints.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
rajeeja
added
bug
cmdupuis3
added
run-benchmark
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have stayed the same:
|
Sevans711
requested changes
Sep 9, 2026
Sevans711
left a comment
Sevans711
left a comment
Collaborator
There was a problem hiding this comment.
These are important user guide improvements, converting the guide to actually use the uxarray implementations of these calculus methods, and explaining contributions from the metric terms which were previously not taught at all. It is definitely moving things in a good direction! I left a few inline comments with suggestions/questions.
Overall design question: would it make more sense to include the formulas all at the top of the page? Now that the formulas are more complicated than just the basic definitions which people might naively expect, having all the basic formulas at the top might make it easier to separate these math surprises from the uxarray syntax, implementation details, and visualizations.
| "\\text{curl}(\\mathbf{F}) = \\frac{\\partial v}{\\partial x} - \\frac{\\partial u}{\\partial y} + \\frac{u \\tan\\varphi}{a}\n", | ||
| "$$\n", | ||
| "\n", | ||
| "That last term comes from the convergence of the meridians and vanishes only on the equator. Dropping it costs a factor of two on solid-body rotation, so `curl()` includes it. Building the same quantity by hand out of `gradient()` components gives the planar answer instead.\n", |
Collaborator
There was a problem hiding this comment.
This is very hard for me to parse, please include more details. Maybe it would also help to link to a paper or website that presents/derives the formula, too?
Notes:
- "convergence of the meridians" - what does this mean?
- "comes from the convergence of the meridians" - how?
- "Dropping it costs a factor of two on solid-body rotation" - why?
- "so
curl()includes it" - this could be removed; it was a bit confusing to read. My default assumption is that the uxarray implementation would match whatever formula is presented here. - "Building the same quantity by hand out of
gradient()components" - what does this mean? - "gives the planar answer instead" - what does "the planar answer" mean?
Contributor
Author
There was a problem hiding this comment.
Rewrote it: the cell now starts from the spherical curl (1/(a cos φ))[∂v/∂λ − ∂(u cos φ)/∂φ] and shows the product rule producing u tan φ/a, with a pointer to Holton and Hakim Ch. 4. Dropped "so curl() includes it" and replaced the "by hand" sentence with the literal gradient() expression it referred to.
Collaborator
There was a problem hiding this comment.
The rewrite makes it much easier to read and understand quickly, thank you!
I followed the pointer and spent ~3 mins scrolling around to try to find the corresponding equation but didn't see it in that amount of time. Could you mention the relevant equation number here, too? Aside from that, this part looks good to me now.
Collaborator
There was a problem hiding this comment.
Now that I know there could be a real error of roughly 5.5e-9 due to the metric term, it is difficult to trust that the 1e-5 error here is "small". Is this error really "small" or is it a sign of a meaningful mismatch between the analytical expectation and the computed results?
Contributor
Author
There was a problem hiding this comment.
Good catch, it was not small, the reference had the wrong sign: −(1/(R cos φ)) ∂(ΩR cos²φ)/∂φ = +2Ω sin φ, i.e. planetary vorticity. Against the correct sign the max error is 8.5e-10 (0.02%), and against the old one every face disagreed in sign. This came in with #1517 and went unnoticed because the notebook stores no outputs; fixed in 3b3399e.
Collaborator
There was a problem hiding this comment.
The current readthedocs preview is now showing:
computed zeta range: [-4.448e-06, 4.259e-06] 1/s
analytic zeta range: [-5.039e-06, 4.844e-06] 1/s
max |error|: 8.523e-10 1/s (≈ 0.02% of max |zeta|)
I don't understand these numbers - to me it looks like the maximum error must be at least 20% of max |zeta|, not 0.02%. For example, max(analytic) minus max(computed) gives 0.6e-06, not 8e-10. What am I missing here, or is there a typo somewhere?
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
3 participants
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 #1731. Follow-up to #1663 / #1662.
Overview
Two things #1663 left behind.
1.
divergenceis eager on dask input. #1663 added the spherical metric term to bothcurlanddivergence. The two are companions and should behave identically, butdivergencereaches forother.valueswherecurlusesself.data:That materializes
otherand drags the whole divergence eager, socurlanddivergencereturn different types for the same chunked input. The mismatch came in when #1588's.values→.datasweep was applied during a conflict resolution on the #1663 branch and only reached thecurlside.Measured on
test/meshfiles/ugrid/quad-hexagon/grid.ncwith a chunked field:curlArrayArraydivergencendarrayArrayNo test added. Nothing in
test/core/test_vector_calculus.pychunks its input, so the whole gradient/curl/divergence area has no dask coverage to extend; adding just enough scaffolding to pin this one line would be out of proportion to a one-word fix. Worth a separate issue if we want that coverage properly. The existing 38 tests pass unchanged.2.
docs/user-guide/vector_calculus.ipynbstill teaches the planar operators. The curl and divergence background cells givewith no metric term, and eight code cells hand-roll the operators out of
gradient()components instead of callingcurl()/divergence(). So every worked example demonstrated pre-#1663 physics while the library computed something else.This adds the$u\tan\varphi/a$ and $-v\tan\varphi/a$ terms to the math, switches the examples to the public API, and corrects the residual magnitudes quoted in the prose to what the notebook actually prints.
One result changes materially. Curl and divergence of a constant field are no longer exactly zero — they are$u\tan\varphi/a$ and $-v\tan\varphi/a$ , which come to
4.8e-09and9.6e-09on this ±2° MPAS subset. That is correct spherical behaviour, so the notebook now says so and checks both against their closed forms (max deviation0.00e+00) rather than dropping the examples. Everything else moves by less than a part in 10⁴, except the two "≈ 0" residuals which go from2.8e-10to4.0e-09and stay ≈ 0.The notebook is executed by
myst_nbat docs build time and stores no outputs, so its printed numbers were always fresh while the prose around them went stale silently. Every cell was run locally before committing.PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rstAI Disclosure
AI Usage: