Skip to content

Phase out InverseRTransform and use forward transforms directly #313

Open
Ao-chuba wants to merge 4 commits into
theochem:masterfrom
Ao-chuba:refactor/deprecate-inverse-rtransform
Open

Phase out InverseRTransform and use forward transforms directly #313
Ao-chuba wants to merge 4 commits into
theochem:masterfrom
Ao-chuba:refactor/deprecate-inverse-rtransform

Conversation

@Ao-chuba

Copy link
Copy Markdown
Member

closes #308.

  • Deprecated InverseRTransform and preserved it purely for backward compatibility with a DeprecationWarning.
  • Refactored solve_poisson_bvp and solve_poisson_ivp to accept forward transforms directly.
  • Refactored solvers in ode.py to consistently operate on the finite integration domain using transform.inverse.
  • Updated internal coordinate mapping to use BaseTransform.deriv_inverse and related methods instead of wrapping.
  • Refactored BVP and IVP tests in test_ode.py to reflect the correct forward transform bounds and ensure strict monotonicity.
  • All internal usages of InverseRTransform have been removed from the codebase.
  • The only lines missing in the coverage are from InverseRTransform, which is deprecated and will be removed soon.

@marco-2023 Since InverseRTransform is not being used no more, would you prefer I leave it deprecated for a release cycle as currently implemented, or should I go ahead and completely remove the class and its unit tests right now in this PR?

do let me know if further changes required.

@Ao-chuba

Copy link
Copy Markdown
Member Author

When call passes InverseRTransform to solve_poisson_bvp, the solver crashed inside _solve_poisson_bvp_atomgrid with: ValueError: The codomain of the transform (-1, 1) should be in [0, infinity).

This happened because InverseRTransform maps its own codomain to the underlying forward transform's domain (which has negative values, e.g. [-1, 1]), failing the non negativity check. To prevent this crash and maintain backward compatibility, we now automatically unpack InverseRTransform into its underlying forward transform at the entry points of solve_poisson_bvp and solve_poisson_ivp.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR continues the #308 migration away from InverseRTransform by deprecating it and updating Poisson/ODE solvers (and their tests) to operate using the forward transform plus BaseTransform’s inverse-derivative helpers.

Changes:

  • Deprecates InverseRTransform (kept for backward compatibility via DeprecationWarning) and updates call sites/tests to pass the forward transform directly.
  • Refactors solve_ode_ivp / solve_ode_bvp and internal ODE transformation helpers to solve on the finite domain using transform.inverse(...) plus deriv_inverse* methods.
  • Updates Poisson solvers and ODE/Poisson tests to match the new forward-transform convention and revised bounds.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/grid/tests/test_poisson.py Updates Poisson tests to pass forward transforms directly (no InverseRTransform).
src/grid/tests/test_ode.py Refactors ODE tests for the new “solve on finite domain” transform convention and uses deriv_inverse*.
src/grid/rtransform.py Adds a DeprecationWarning and deprecation docs to InverseRTransform.
src/grid/poisson.py Updates Poisson solver docs/validation for forward transforms and adds backward-compat unwrap for InverseRTransform.
src/grid/ode.py Refactors ODE solvers/helpers to use transform.inverse and inverse-derivative methods consistently.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/grid/poisson.py Outdated
Comment thread src/grid/poisson.py Outdated
Comment thread src/grid/ode.py
Comment thread src/grid/ode.py
Comment thread src/grid/ode.py
@Ao-chuba

Copy link
Copy Markdown
Member Author

updated the pr,
cc @PaulWAyers @marco-2023
if any update required please do let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

We need to phase out InverseRTransform

2 participants