Repository navigation
Fix HandyModRTransform.deriv3 for m > 2 - #348
Merged
Merged
Conversation
The first term of the numerator in deriv3 used 2 * 2**m where the analytic third derivative has (2**m)**2. That term is multiplied by (m - 1)(m - 2), so it vanishes for m = 1 and m = 2, which is why the existing tests (all with m <= 2) did not catch it. For m >= 3 deriv3 is wrong, and so are deriv3_inverse and solve_ode_ivp/solve_ode_bvp with this transform on third-order ODEs.
Collaborator
|
Thanks, @SajalDevX, for your contribution. I added an analytical test and fixed the bounds of your test. |
marco-2023
self-requested a review
November 5, 2026 12:59
marco-2023
approved these changes
Nov 5, 2026
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 #347
HandyModRTransform.deriv3had2 * two_min the first term of the numerator where the analytic third derivative hastwo_m**2. That term is multiplied by(m - 1)(m - 2), so it only goes wrong form >= 3, and the existing tests all usem <= 2. The bad value also propagates toderiv3_inverseand tosolve_ode_ivp/solve_ode_bvpon third-order ODEs with this transform (e.g.y''' = 6withm = 3was about 10% off the exact(x + 0.5)**3; it now matches).Changes:
rtransform.py:2 * two_m->two_m**2inHandyModRTransform.deriv3.test_transform.py:test_handymod_deriv3_matches_finite_differences, checkingderiv3against the existing finite-difference helper for m = 1 to 5.Testing:
deriv3against the sympy third derivative oftransformfor m = 1 to 5 (relative error ~1e-15 after the fix), and reran thesolve_ode_ivpexample from the issue.pytest src/grid/tests/test_transform.py src/grid/tests/test_rtransform.py src/grid/tests/test_ode.pypasses (317 passed).I used Claude to help find the bug and draft the test; I reviewed the change and ran the tests myself.