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

Improved Integrators #300

Merged
merged 19 commits into from
Feb 7, 2025
Merged

Improved Integrators #300

merged 19 commits into from
Feb 7, 2025

Conversation

LarsKue
Copy link
Contributor

@LarsKue LarsKue commented Feb 6, 2025

This PR:

The current code for integration is still a little messy due to lack of proper encapsulation. However, we can update this as we go without much impact to the user, if any.

I tested this running Flow Matching on the Two Moons example, and on some toy examples, both in non-compiled and compiled contexts.

@LarsKue LarsKue added the feature New feature or request label Feb 6, 2025
@LarsKue LarsKue self-assigned this Feb 6, 2025
@paul-buerkner paul-buerkner added this to the BayesFlow 2.0 milestone Feb 6, 2025
@LarsKue LarsKue marked this pull request as draft February 6, 2025 14:48
@stefanradev93
Copy link
Contributor

That already looks really good! Which integrator do you think should we use as a default or should I run some experiments?

@LarsKue
Copy link
Contributor Author

LarsKue commented Feb 7, 2025

@stefanradev93 In my tests, RK45 runs in slightly fewer function evaluations with dynamic step size on. For a static step size, I would still choose Euler, since Flow Matching tends to have straight trajectories. But we can leave RK45 as the default, I think, or even implement more algorithms.

That said, there is still an issue somewhere, presumably with negative step sizes (when start_time > stop_time). I will mark the PR as ready for review again when I have fixed this.

fix small deviations in some backends
add (required) min and max step number selection for dynamic integration
improve dispatch
remove step size selection (users can compute this if they need it, but exposing the argument is ambiguous w.r.t. fixed vs adaptive integration)
add default integrate kwargs
@LarsKue LarsKue marked this pull request as ready for review February 7, 2025 16:59
@stefanradev93 stefanradev93 merged commit 9071be4 into dev Feb 7, 2025
13 checks passed
@stefanradev93 stefanradev93 deleted the integrators branch February 7, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants