Add unit tests for convection_terms.py#2025
Closed
irhyl wants to merge 1 commit intogoogle-deepmind:mainfrom
Closed
Add unit tests for convection_terms.py#2025irhyl wants to merge 1 commit intogoogle-deepmind:mainfrom
irhyl wants to merge 1 commit intogoogle-deepmind:mainfrom
Conversation
Create convection_terms_test.py with 17 tests for make_convection_terms, covering: - Zero convection produces zero mat/vec - Output shape verification - Tridiagonal matrix structure - Single-cell grid NotImplementedError - All three Dirichlet modes (ghost, direct, semi-implicit) - Both Neumann modes (ghost, semi-implicit) - Invalid mode ValueError - Dirichlet BC contributions to vec - Zero-gradient Neumann behavior - Negative diffusion coefficient handling - Large/small Peclet number limits (convection/diffusion dominated) Bug: b/469726859
Nush395
reviewed
Mar 27, 2026
Collaborator
Nush395
left a comment
Nush395
left a comment
There was a problem hiding this comment.
Thanks for the contribution, I've made comments pertaining to the removal/tidying up of some of the added tests.
| @@ -0,0 +1,324 @@ | |||
| # Copyright 2025 DeepMind Technologies Limited | |||
|
|
||
|
|
||
| class ConvectionTermsTest(parameterized.TestCase): | ||
| """Tests for make_convection_terms.""" |
Collaborator
There was a problem hiding this comment.
docstring here is unnecessary
| """Tests for make_convection_terms.""" | ||
|
|
||
| def test_zero_convection_returns_zero_mat_and_vec(self): | ||
| """When v_face is zero everywhere, mat and vec should be zero.""" |
Collaborator
There was a problem hiding this comment.
this docstring does not add any useful information, it could be removed or moved to be a comment on line 41 e.g.
| np.testing.assert_allclose(mat, np.zeros((4, 4)), atol=1e-12) | ||
| np.testing.assert_allclose(vec, np.zeros(4), atol=1e-12) | ||
|
|
||
| def test_output_shapes(self): |
Collaborator
There was a problem hiding this comment.
shape tests are done in other tests you have added so this test can be removed.
| v_face=jnp.ones(5), | ||
| d_face=jnp.ones(5), | ||
| var=cell_var, | ||
| dirichlet_mode='invalid', |
Collaborator
There was a problem hiding this comment.
We can change the type of the call signature of dirichlet_mode to be a Literal of the allowed string types and then this test can be removed as any incorrect call would be picked up by static type analysis.
| dirichlet_mode='invalid', | ||
| ) | ||
|
|
||
| def test_invalid_neumann_mode_raises(self): |
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.
Summary
Addresses
TODO(b/469726859)by adding the first dedicated test file forconvection_terms.py, which previously had zero unit test coverage. Tests cover all boundary condition modes, edge cases, and Peclet number limiting behavior.Changes
torax/_src/fvm/tests/convection_terms_test.py- 17 tests inConvectionTermsTestdiffusion_terms_test.py_make_face_centershelper for uniform grid constructionTest coverage
NotImplementedErrorValueErrorTesting
All 17 new tests pass.
Bug: b/469726859