ENH: Extend the echo top impermeability condition to all engines - #165
Merged
Conversation
PR openradar#164 added an integer upper_bc with a new mode 2, which holds w fixed above the echo top, but only implemented it for the mass continuity term of the scipy engine. This extends it to every gradient term of the scipy, jax, tensorflow and auglag engines. The set of points at which the condition applies is now calculated once per retrieval by the new calculate_echo_top_mask and stored on DDParameters.upper_bc_mask, rather than recomputing an (n_radar, nz, ny, nx) reduction inside the gradient on every iteration. Each gradient function takes a matching upper_bc_mask keyword and applies it through a small per-module _apply_upper_bc helper. Also fixed along the way: * Every term guarded its upper boundary condition with `upper_bc is True`. Since `1 is True` is False, passing upper_bc=1 (as the TWP-ICE tests and examples/plot_examples.py already did) silently disabled the grid top condition. All terms now compare with ==, so both the integer modes and the legacy booleans work. * The jax vertical vorticity gradient discarded the result of `w_grad.at[...].set(0)`, so it never applied the condition at all. * grad_J passed upper_bc to calculate_point_gradient, which does not accept it, raising a TypeError whenever Cpoint > 0. The same applied to grad_background_cost in the auglag module. The point term's w gradient is identically zero, so the condition is a no-op there and the argument is dropped. auglag's grad_smooth_cost was also missing dx, dy and dz. Documents the three modes and the `above` parameter in the docstrings and in a new section of the optimizing wind retrieval user guide, and adds cost function and TWP-ICE retrieval tests for each engine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The PR openradar#164 merge crossed the two sides of the parallel mass continuity gradient call: it kept main's calculate_mass_continuity_gradient signature, which has no vrs parameter, alongside the branch's call site, which still passed parameters.vrs. That sent 12 positional arguments into a function taking 7 to 11, raising TypeError. Removing the argument is the right direction rather than restoring the parameter: the branch's per-gradient vrs/above arguments were consolidated into calculate_echo_top_mask(vrs, z, above), whose result is threaded through as the precomputed upper_bc_mask. The matching above= argument had already been dropped from this call; only vrs survived. Only the scipy parallel path was affected, which is why the jax and tensorflow variants passed. parallel=True is the default, so the tests that do not mention it failed too. The other seven gradient calls in the same block were checked against their signatures and their arities match. Fixes the six test_retrieval.py failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 70.45% 71.28% +0.83%
==========================================
Files 32 32
Lines 5361 5509 +148
==========================================
+ Hits 3777 3927 +150
+ Misses 1584 1582 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
PR #164 added a feature to implement a boundary condition with impermeable cloud tops (upper_bc=2). It was also noted that there were issues where the upper boundary condition was never set when upper_bc=1. These are now fixed.
I used Claude Opus 5.