Upper bc addition for impermeable cloud top - #164
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
- Coverage 70.49% 70.45% -0.05%
==========================================
Files 32 32
Lines 5355 5361 +6
==========================================
+ Hits 3775 3777 +2
- Misses 1580 1584 +4
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:
|
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
I had Claude add the pre-commit hooks to lint your code for formatting and push those changes to your branch. PyDDA uses pre-commit hooks to ensure consistent style throughout the code, so I would, for future PRs, consider installing pre-commit in your repository so that these checks run automatically. |
|
Thanks Bobby, looks like it was mostly formatting style things. pre_commit now installed! |
|
Another recommendation I have is to add a unit test where we test the upper_bc=2 option so that this code is tested and we know it works. Codecov is failing since the new additions are not unit tested. You can even use the examples here and add a module that will run the TWP-ICE example with upper_bc=2, checking against expected values for your PyDDA upper_bc=2 for the TWP-ICE example. The TWP-ICE code is here in test_twpice_case: |
|
Adding tests + capabilities to Jax/Tensorflow will be another PR. Merging. |
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>
PR #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>
2 participants
This PR implements the upper boundary condition modifications discussed for the retrieval workflow. It applies in the scipy option ONLY, this change moves the upper_bc parameter to an integer, making the meanings 0 = no condition, 1 = impermeable at grid top, 2 = impermeable at observation top. It also adds a new parameter "above" which defines a minimum altitude to apply this at.
As in https://doi.org/10.5194/egusphere-2026-4631