Refactor transfer functions to handle division by zero and NaN values#588
Merged
selipot merged 3 commits intoCloud-Drift:mainfrom Jan 2, 2026
Merged
Refactor transfer functions to handle division by zero and NaN values#588selipot merged 3 commits intoCloud-Drift:mainfrom
selipot merged 3 commits intoCloud-Drift:mainfrom
Conversation
- Added np.errstate context managers to suppress warnings and handle divisions by zero in various transfer functions. - Updated the calculation of the `zo` variable to return np.inf when mu is zero to avoid division by zero errors. - Modified the `_xis` function to return NaN for non-finite values. - Enhanced unit tests to ensure proper handling of edge cases and validate output shapes for both no-slip and free-slip boundary conditions. - Renamed test methods for clarity and consistency with the new transfer function implementations.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors transfer functions in clouddrift/transfer.py to properly handle division by zero and NaN values, addressing warnings identified in issue #582. The changes add comprehensive error state management and improve test coverage for edge cases.
- Added
np.errstatecontext managers throughout transfer functions to suppress division and invalid operation warnings - Modified
zocalculation to explicitly returnnp.infwhenmu == 0to avoid division by zero - Updated
_xisfunction to replace non-finite values with NaN for proper propagation - Enhanced test suite with expanded coverage for both "lilly" and "elipot" methods across no-slip and free-slip boundary conditions
- Uncommented and updated
TransferFunctionTestMethodsclass to validate method equivalence
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| clouddrift/transfer.py | Added np.errstate context managers across multiple transfer functions; modified zo calculations to handle mu == 0 explicitly; updated _xis to convert non-finite values to NaN |
| tests/transfer_test.py | Renamed test methods for clarity; added comprehensive test coverage for both methods and boundary conditions; uncommented TransferFunctionTestMethods class; updated assertions to handle NaN values with equal_nan=True |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+900
to
+958
| for s in [1, -1]: | ||
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | ||
| for j, bld in enumerate(self.bld): | ||
| Ge, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="elipot", | ||
| boundary_condition=self.slipstr1, | ||
| ) | ||
| Gl, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="lilly", | ||
| boundary_condition=self.slipstr1, | ||
| ) | ||
| # bool_idx = Ge != np.nan and Gl != np.nan | ||
| # print(Ge[bool_idx], Gl[bool_idx]) | ||
| print(Ge, Gl) | ||
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | ||
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | ||
| self.assertTrue(bool1) | ||
|
|
||
| def test_method_equivalence_free_slip(self): | ||
| for s in [1, -1]: | ||
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | ||
| for j, bld in enumerate(self.bld): | ||
| Ge, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="elipot", | ||
| boundary_condition=self.slipstr2, | ||
| ) | ||
| Gl, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="lilly", | ||
| boundary_condition=self.slipstr2, | ||
| ) | ||
| # bool_idx = Ge != np.nan and Gl != np.nan | ||
| # print(Ge, Gl) | ||
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | ||
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | ||
| self.assertTrue(bool1) |
There was a problem hiding this comment.
The loop variable 's' is defined but never used inside the loop. If this variable is intended to test with different signs of the Coriolis frequency, it should be passed to the wind_transfer function (e.g., as s * self.cor_freq). Otherwise, this loop iteration is redundant.
Suggested change
| for s in [1, -1]: | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge[bool_idx], Gl[bool_idx]) | |
| print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) | |
| def test_method_equivalence_free_slip(self): | |
| for s in [1, -1]: | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge[bool_idx], Gl[bool_idx]) | |
| print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) | |
| def test_method_equivalence_free_slip(self): | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) |
Comment on lines
+900
to
+958
| for s in [1, -1]: | ||
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | ||
| for j, bld in enumerate(self.bld): | ||
| Ge, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="elipot", | ||
| boundary_condition=self.slipstr1, | ||
| ) | ||
| Gl, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="lilly", | ||
| boundary_condition=self.slipstr1, | ||
| ) | ||
| # bool_idx = Ge != np.nan and Gl != np.nan | ||
| # print(Ge[bool_idx], Gl[bool_idx]) | ||
| print(Ge, Gl) | ||
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | ||
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | ||
| self.assertTrue(bool1) | ||
|
|
||
| def test_method_equivalence_free_slip(self): | ||
| for s in [1, -1]: | ||
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | ||
| for j, bld in enumerate(self.bld): | ||
| Ge, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="elipot", | ||
| boundary_condition=self.slipstr2, | ||
| ) | ||
| Gl, _, _ = wind_transfer( | ||
| self.omega, | ||
| self.z, | ||
| self.cor_freq, | ||
| delta, | ||
| mu, | ||
| bld, | ||
| method="lilly", | ||
| boundary_condition=self.slipstr2, | ||
| ) | ||
| # bool_idx = Ge != np.nan and Gl != np.nan | ||
| # print(Ge, Gl) | ||
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | ||
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | ||
| self.assertTrue(bool1) |
There was a problem hiding this comment.
The loop variable 's' is defined but never used inside the loop. If this variable is intended to test with different signs of the Coriolis frequency, it should be passed to the wind_transfer function (e.g., as s * self.cor_freq). Otherwise, this loop iteration is redundant.
Suggested change
| for s in [1, -1]: | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge[bool_idx], Gl[bool_idx]) | |
| print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) | |
| def test_method_equivalence_free_slip(self): | |
| for s in [1, -1]: | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr1, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge[bool_idx], Gl[bool_idx]) | |
| print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) | |
| def test_method_equivalence_free_slip(self): | |
| for i, (delta, mu) in enumerate(zip(self.delta, self.mu)): | |
| for j, bld in enumerate(self.bld): | |
| Ge, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="elipot", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| Gl, _, _ = wind_transfer( | |
| self.omega, | |
| self.z, | |
| self.cor_freq, | |
| delta, | |
| mu, | |
| bld, | |
| method="lilly", | |
| boundary_condition=self.slipstr2, | |
| ) | |
| # bool_idx = Ge != np.nan and Gl != np.nan | |
| # print(Ge, Gl) | |
| # bool1 = np.allclose(Ge[bool_idx], Gl[bool_idx], atol=1e-8) | |
| bool1 = np.allclose(Ge, Gl, atol=1e-8, equal_nan=True) | |
| self.assertTrue(bool1) |
tests/transfer_test.py
Outdated
Comment on lines
+889
to
+891
| self.bld = [100.0, 200.0, 200.0, np.inf] | ||
| self.K0 = [1 / 10, 1 / 20, 1 / 30] | ||
| self.K1 = [1, 2, 3] |
There was a problem hiding this comment.
The self.bld list has 4 elements [100.0, 200.0, 200.0, np.inf], but self.K0 and self.K1 only have 3 elements each. This could lead to unexpected test behavior in the nested loops where all combinations are tested. Consider ensuring that the lists have compatible lengths or clarifying the intended test combinations.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
clouddrift/transfer.py
Outdated
| """ | ||
|
|
||
| zo = np.divide(delta**2, mu) | ||
| zo = np.inf if mu == 0 else np.divide(delta**2, mu) |
Contributor
There was a problem hiding this comment.
otherwise, if they are arrays, we can do:
zo = np.divide(delta**2, mu, out=np.full_like(mu, np.inf), where=mu != 0)
philippemiron
approved these changes
Jan 2, 2026
Member
Author
|
Thanks @philippemiron, I have now added more consistent references throughout |
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
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.
Fixes #582
zovariable to return np.inf when mu is zero to avoid division by zero errors._xisfunction to return NaN for non-finite values.