Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix complex-valued t_span for complex-valued ODEs in odeint #179

Closed
wants to merge 5 commits into from
Closed

Fix complex-valued t_span for complex-valued ODEs in odeint #179

wants to merge 5 commits into from

Conversation

gautierronan
Copy link
Contributor

@gautierronan gautierronan commented Feb 1, 2023

Following up on my PR #178 and on this issue #177.

In the current state of odeint, the time values t are complex-valued if x is initially complex-valued when calling f(t, x) at every time step. Instead, they should be float like. Otherwise, this could be problematic if f(t, x) requires real-valued times (e.g. if f(t, x) = torch.erf(t) * x).

This PR fixes this by making changes to the solver.tableau definitions. The c constants in the tableaus are initialized with a float dtype of the same precision as the (complex or float) dtype of x. This is done by calling t_dtype = getattr(torch, torch.finfo(x.dtype).dtype). If x is float-valued, then t_dtype = x.dtype and things work as usual. Changes in sync_device_dtype were also required to differentiate between the t.dtype and the x.dtype.

Sorry for having missed this in the initial PR !

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Base: 62.53% // Head: 62.63% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (c068b2b) compared to base (201026a).
Patch coverage: 90.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   62.53%   62.63%   +0.09%     
==========================================
  Files          27       27              
  Lines        1879     1884       +5     
==========================================
+ Hits         1175     1180       +5     
  Misses        704      704              
Impacted Files Coverage Δ
torchdyn/numerics/solvers/ode.py 53.12% <50.00%> (+0.21%) ⬆️
torchdyn/numerics/sensitivity.py 97.95% <100.00%> (ø)
torchdyn/numerics/solvers/_constants.py 100.00% <100.00%> (ø)
torchdyn/numerics/solvers/templates.py 75.00% <100.00%> (+0.58%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gautierronan
Copy link
Contributor Author

gautierronan commented Feb 3, 2023

Actually, thinking more on this, I'm not sure what's the best practice. Should tspan stay complex-valued to keep every object in the model of the same type? Or should it be real-valued for consistency?

I'm unsure of what's best... I'll let the maintainers decide!

@Zymrael
Copy link
Member

Zymrael commented Feb 27, 2023

Thanks for contributing! It makes sense to keep t real-valued. We don't really have any particular use case at the moment where it becomes convenient to keep x and t of the same dtype.

@gautierronan gautierronan closed this by deleting the head repository Mar 17, 2023
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.

3 participants