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

Default CFL to 0.999 instead of 0.7 #2940

Merged
merged 5 commits into from
Mar 10, 2022
Merged

Conversation

MaxThevenet
Copy link
Member

@MaxThevenet MaxThevenet commented Mar 8, 2022

This PR proposes to:

  • update the documentation (currently, the warpx.cfl parameter is not labeled as optional)
  • Change the default value (from 0.7 currently to 0.999)

My impression is that the second point would make a better default, but that changes the default which could affect users, so I am opening this PR to launch the discussion.

@MaxThevenet MaxThevenet added changes input scripts / defaults Changes the syntax or meaning of input scripts and/or defaults component: core Core WarpX functionality component: documentation Docs, readme and manual labels Mar 8, 2022
Source/WarpX.H Outdated
@@ -1281,7 +1281,7 @@ private:

int regrid_int = -1;

amrex::Real cfl = amrex::Real(0.7);
amrex::Real cfl = amrex::Real(0.99);
Copy link
Member

Choose a reason for hiding this comment

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

Just in case: I usually use 0.999 for all my sims, which is a bit closer not leaving 1% on the table for time steps and still far from machine precision to be rounded to 1.

Suggested change
amrex::Real cfl = amrex::Real(0.99);
amrex::Real cfl = amrex::Real(0.999);

I base this on Figure 2.11 in https://doi.org/10.5281/zenodo.3266820 & Figure 2.10 in https://doi.org/10.5281/zenodo.15924, where I still find the difference is signficant for the third digit.

Docs/source/usage/parameters.rst Outdated Show resolved Hide resolved
@ax3l
Copy link
Member

ax3l commented Mar 8, 2022

Do we want to merge this into #2941, so we only need to reset the checksums once?

@MaxThevenet
Copy link
Member Author

We could reset the benchmark, or explicitly set the CFL to 0.7 in the few tests that were failing, to avoid breaking them. I implemented the latter, but I don't mind either way.

@ax3l ax3l changed the title [mini] default CFL to 0.99 instead of 0.7, and update doc Default CFL to 0.999 instead of 0.7 Mar 9, 2022
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you, good change! :)

@ax3l ax3l self-assigned this Mar 10, 2022
@ax3l ax3l enabled auto-merge (squash) March 10, 2022 22:58
@ax3l ax3l disabled auto-merge March 10, 2022 22:58
@ax3l ax3l enabled auto-merge (squash) March 10, 2022 22:58
@ax3l ax3l merged commit 77af949 into ECP-WarpX:development Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes input scripts / defaults Changes the syntax or meaning of input scripts and/or defaults component: core Core WarpX functionality component: documentation Docs, readme and manual
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants