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

Using PreallocationTools.jl #173

Merged
merged 4 commits into from
Nov 6, 2023
Merged

Using PreallocationTools.jl #173

merged 4 commits into from
Nov 6, 2023

Conversation

jpfairbanks
Copy link
Member

@ChrisRackauckas, I added some fixes for #171

ChrisRackauckas and others added 4 commits November 3, 2023 17:58
…cache

This fixes automatic differentiation, thus allowing implicit methods to be used. Also, this allows for setting the u-type for the caches, thus allowing for automatic differentiation of the solve itself for gradient-based optimization. Demonstrations of use cases will follow, but for now I have added a test showing that implicit methods work and the u-type stuff has been demonstrated locally.
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e321be6) 74.42% compared to head (3d039f3) 74.44%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   74.42%   74.44%   +0.02%     
==========================================
  Files           9        9              
  Lines        1122     1127       +5     
==========================================
+ Hits          835      839       +4     
- Misses        287      288       +1     
Files Coverage Δ
src/Decapodes.jl 100.00% <ø> (ø)
src/simulation.jl 90.75% <96.77%> (-0.16%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisRackauckas
Copy link
Contributor

Awesome, yeah there was just a few dangling things there 😅. Examples of calibration to come when I get in-flight wifi up.

@ChrisRackauckas
Copy link
Contributor

This looks good to me.

(_, :DualForm0, 1) => :E
(_, :DualForm1, 1) => :V
_ => return :AllocVecCall_Error
resolved_form = @match (c.form, c.dimension) begin
Copy link
Member

Choose a reason for hiding this comment

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

PR #151 also makes this refactoring and pulls it into a function to be called elsewhere, but I will leave this be and fix the merge conflict when it is time to merge 151.

@lukem12345 lukem12345 added the enhancement New feature or request label Nov 6, 2023
Copy link
Member

@lukem12345 lukem12345 left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Just reran the CISM benchmark on this branch on HPG, and times are sitting at just over 11 [ms] using the Tsit5 solver. Loading up CairoMakie now to visually inspect results as final sanity check, though RMS and max difference appear normal. Will approve pending the render.

@lukem12345
Copy link
Member

lukem12345 commented Nov 6, 2023

I should note that I was unable to get the parallel solver KuttaPRK2p5 to work due to ERROR: @threads :static cannot be used concurrently or nested. But since the handful of implicit solvers that I tried work, this shouldn't block this PR.

@lukem12345
Copy link
Member

lukem12345 commented Nov 6, 2023

Looks like artifiacting builds up in strange ways using the FBDF solver. Tsit5 results are included for comparison. This is very likely not a "bug" in the solver implementation nor in Decapodes per se. But just thought that it should be noted.

Let me know your thoughts @jpfairbanks and I will go ahead and merge.

FBDF

fdf5

Tsit5

tsit5

@jpfairbanks jpfairbanks merged commit 5a6dd3e into main Nov 6, 2023
9 checks passed
@lukem12345 lukem12345 mentioned this pull request Nov 6, 2023
@ChrisRackauckas
Copy link
Contributor

That goes away at lower tolerances?

@lukem12345
Copy link
Member

Presumably yeah. That's been my experience using the explicit timestepping solvers with Decapdoes. I'll run some more tests with FBDF this afternoon and open an issue if changing up the parameters doesn't resolve this. The GIFs here are using just the default parameters.

@jpfairbanks
Copy link
Member Author

The artifacts shouldn't hold up the merge. We can diagnose the numerics in a separate issue.

@ChrisRackauckas
Copy link
Contributor

Yes this is somewhat expected. For explicit RK methods, the stability bound is stronger than the convergence bound, and therefore the default behavior that you'll get on this PDE has very low error because most of its step failures are due to stability criteria. Meanwhile for the implicit methods, it's only going to be bound by error, and therefore it's going to be hitting a much higher error by default because it's only aiming for the reltol=1e-3 default, which is effectively 1-2 digits of accuracy. That's just a default of "the plot should look good enough" but if you want to remove say the pseudowaves from there that's just a phenomena of numerical advection that can be dampened by using lower tolerances.

@jpfairbanks jpfairbanks deleted the jpf/prealloc branch November 25, 2023 13:46
@ChrisRackauckas
Copy link
Contributor

I should note that I was unable to get the parallel solver KuttaPRK2p5 to work due to ERROR: @threads :static cannot be used concurrently or nested. But since the handful of implicit solvers that I tried work, this shouldn't block this PR.

That one probably won't be a good idea for this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants