TST/FIX: Expand cost function test coverage and fix numpy smoothness gradient#158
Closed
rcjackson wants to merge 5 commits intoopenradar:mainfrom
Closed
TST/FIX: Expand cost function test coverage and fix numpy smoothness gradient#158rcjackson wants to merge 5 commits intoopenradar:mainfrom
rcjackson wants to merge 5 commits intoopenradar:mainfrom
Conversation
…ngines - Add _make_radvel_inputs() helper for building single-radar test inputs - Add test_calculate_rad_velocity_cost_nonzero_jax: verifies JAX cost function and gradient match numpy reference with float32 tolerance - Add test_calculate_rad_velocity_cost_nonzero_tf: same for TensorFlow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
For each cost function (smoothness, mass continuity, background, vertical vorticity, model, point), adds nonzero-field tests for both JAX and TensorFlow engines that verify: - Cost values match the numpy reference within float32 tolerance - Gradients are nonzero (or match numpy where implementations agree) Notable findings documented in test comments: - Smoothness gradient: numpy omits Cx/Cy/Cz coefficients and uses scipy.ndimage.laplace; cross-engine comparison is not meaningful - Mass continuity JAX gradient: jax.vjp differs from numpy's analytic adjoint at grid boundaries; cost comparison is used instead - Vertical vorticity TF: default coeff=1 vs numpy/JAX default coeff=1e-5; explicit coeff=1e-5 is now passed in tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous implementation used scipy.ndimage.laplace (biharmonic operator) without the Cx, Cy, Cz weighting coefficients, producing a gradient inconsistent with the actual cost function. Replace with the analytic gradient of the cost: grad = 2*Cx * d²fx/dx² + 2*Cy * d²fy/dy² + 2*Cz * d²fz/dz² where fx/fy/fz are the combined second-derivative terms from the cost. u, v, w share the same gradient since the cost treats them symmetrically. Also remove unused scipy and _nd_image imports, and strengthen the smoothness gradient tests to do full cross-engine numerical comparison now that all three engines are consistent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Claude, you silly goose...not committing. |
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.
calculate_smoothness_gradient: the previous implementation usedscipy.ndimage.laplacewithout the Cx/Cy/Cz coefficients, producing a gradient inconsistent with the actual cost function. Replaced with the correct analytic gradient usingnp.gradient. Removes unused scipy imports.