Skip to content

Remove backtracking, add more caching - #2359

Merged
visr merged 16 commits into
mainfrom
custom_line_search
Jun 11, 2025
Merged

Remove backtracking, add more caching#2359
visr merged 16 commits into
mainfrom
custom_line_search

Conversation

@SouthEndMusic

@SouthEndMusic SouthEndMusic commented Jun 5, 2025

Copy link
Copy Markdown
Collaborator

Fixes #2189
Fixes #1778

@visr

visr commented Jun 6, 2025

Copy link
Copy Markdown
Member

Unless I messed up something in my benchmarking, it seems that the HWS time profile still looks about the same as #2330 (comment), but the allocation situation has improved, going from ~90% to ~70% in relaxation.

The issue with the profiler saying there is GC and dynamic dispatch around the closure remains. Still I would've expected a speedup if this does less iterations. Instead I see a 2x regression that I don't understand. At least a part of that is due to now running with check-bounds=yes, so not completely fair. This is still a bit of a mystery to me. Great to have progress on this front though.

@SouthEndMusic SouthEndMusic changed the title Custom line search that doesn't throw at failure Remove backtracking, add more caching Jun 10, 2025
@SouthEndMusic

SouthEndMusic commented Jun 10, 2025

Copy link
Copy Markdown
Collaborator Author

I've been fiddling around quite a bit in this branch, but in the end:

  • Backtracking was removed completely, as it didn't do anything other than take time
  • Most time interpolations are now cached. I introduced a TimeDependentCache for that, and renamed the existing DiffCache StateTimeDependentCache, to make more clear what it is. Lastly I renamed ParametersNonDIff to ParametersIndependent for consistency. There are now several parts in water_balance! where computations are only done when the data they depend on is new, whether that is the state u, the time t or both.

@SouthEndMusic
SouthEndMusic requested a review from visr June 10, 2025 11:00

@visr visr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good overall. I still want to run some benchmarks. And probably good to make an issue to look into adding relaxation again in the future?

Comment thread core/src/Ribasim.jl Outdated
Comment thread core/src/util.jl
Comment thread core/src/util.jl
@SouthEndMusic

Copy link
Copy Markdown
Collaborator Author

I don't understand what went wrong in CI, I cannot reproduce the failing minimal_subnetwork model or the sparse and AD/FDM jac solver options tests locally

@visr

visr commented Jun 10, 2025

Copy link
Copy Markdown
Member

Restarting CI fixed one of them, the other timed out. It isn't clear to me why CI takes so much longer on this branch, and why there are spurious failures. That makes me a bit hesitant to merge this as one. Perhaps we should first remove relaxation separately?

@visr visr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm I see the CI time regression also on main, so this branch doesn't seem responsible for it. Great, then we can merge it at once.

@visr
visr merged commit b9bc641 into main Jun 11, 2025
@visr
visr deleted the custom_line_search branch June 11, 2025 07:53
@visr visr mentioned this pull request Jun 12, 2025
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.

Limit backtracking iterations Get rid of OrdinaryDiffEq internals usage

2 participants