-
Notifications
You must be signed in to change notification settings - Fork 3
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
Use FastBroadcast.jl #143
Use FastBroadcast.jl #143
Conversation
Simulations could be much faster if SciML/RecursiveArrayTools.jl#400 was resolved... |
@JoshuaLampert Do you know why you set the lower compat of RecursiveArrayTools.jl to 0.3.3 in #118? This is involved in compatibility issues on Julia 1.9... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
=======================================
Coverage 97.75% 97.76%
=======================================
Files 22 23 +1
Lines 1741 1747 +6
=======================================
+ Hits 1702 1708 +6
Misses 39 39 ☔ View full report in Codecov by Sentry. |
Thanks a lot! The performance improvement already looks quite nice.
Can you appraise whether the
Do you have an idea where we loose the remaining 5 to 25%? Do you recommend using FastBroadcast.jl for the other equations, too? Probably the performance gain would not be too high because the solving the elliptic equations is the most expensive, but I take what I can get. If you think it could help, I could do that in a future PR.
I don't remember in detail, but judging from the commit history in that PR and the respective CI runs, it looks like it was because RecursiveArrays.jl >v3.3 doesn't support Julia v1.9. This comes from SciML/RecursiveArrayTools.jl#324, which was included in v3.4 of RecursiveArrays.jl. |
This, of course, only explains why I did't choose it to be higher. I don't remember why I didn't choose it to be lower. |
At least the modified energy/entropy allocates a full space vector every time it is called |
We compute the derivative of the bathymetry |
As you said, I expect the elliptic solve(s) to dominate significantly for the other equations. Maybe it could help a bit but I would not expect significant performance gains like here |
Co-authored-by: Joshua Lampert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Closes #126
Some performance benchmarks similar to #135 (comment)
Before
With FastBroadcast.jl:
To get values that are closer to our paper, we have to omit the analysis callback (which allocates quite a lot):
However, this is still significantly slower (between 5% and 25%).