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

CFL Condition: Account for Rotation Rate #348

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

feathern
Copy link
Contributor

@feathern feathern commented Dec 11, 2021

This PR modifies the CFL calculation in Rayleigh to account for the rotation rate. The timestep may now be no larger than CFLMax/(c1*4), where CFLMax is the CFL safety factor. c1 is the 1st Rayleigh constant, effectively the inverse of the rotational timescale (c1 = 2 Omega for reference_type=2 and 2/Ek for reference_type=1).

@feathern
Copy link
Contributor Author

feathern commented Dec 11, 2021

The test failed because the new CFL is more stringent than the time step that we've been setting in the benchmark file. The numbers measured at the end of 400 timesteps change as a result. I will think about the best way to address this and update the PR.

added benchmark_mode constraint to rotational CFL calculation
@feathern
Copy link
Contributor Author

Hold off a little on accepting this until I confirm. I'm checking with a few people to see if this is a reasonable default (dt = 0.075 E). If anyone sees this and has thoughts on it -- do comment here!

@orvedahl
Copy link
Contributor

When I run Boussinesq MHD models, I consistently had to use dt <= 0.1 * Ek, so I would say 0.075 is a perfectly reasonable default

@feathern
Copy link
Contributor Author

OK @orvedahl . This sounds reasonable to a few others as well, so we'll go with it. Go ahead and merge if you're good with this.

Copy link
Contributor

@orvedahl orvedahl left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks Nick.

@orvedahl orvedahl merged commit 5f34f5d into geodynamics:master Dec 13, 2021
@nschaeff
Copy link

nschaeff commented Dec 14, 2021

For what it's worth, XSHELLS uses dt <= 0.3 Ek as rotation constraint on the time-step for the CNAB time-scheme [1]. Ran many tests and never seemed to be an issue. Note however than cheb may require a more stringent criterium than FD.
If you are also using CNAB, I think you are way too conservative with dt <= 0.1Ek, effectively reducing time-to-solution by a factor 3 when this criterion dominates the time-step selection...
Also, I fail to understand @orvedahl when he says! "I use dt <= 0.1 Ek, so Rayleigh should use dt <= 0.075 Ek". Is it to make the default slower than needed? Or is it an intel-MKL-like strategy where it will run slower for non-specialists? (just kidding).

[1] https://bitbucket.org/nschaeff/xshells/src/d4e44d483a70685bd4868bc82a1a6ab8ee5cf21f/xshells_stepper.cpp#lines-216

@orvedahl
Copy link
Contributor

@nschaeff Nick was asking if the default (0.075 Ek) seemed reasonable, I was simply adding my opinion based on results from my simulations. In my experience with Rayleigh, the timestep often needed to be restricted to less than 0.1 Ek for my moderately expensive runs, any higher and the simulation would come to a stop (usually because the velocities grew exponentially, causing the timestep to decrease below the minimum threshold). I spent a lot of time trying to track down why this was the case, e.g., spatial resolution, temporal resolution, existence of sharp features, etc. The fastest solution for me was always a slightly smaller timestep.

I am not saying "I use this dt, therefore everyone else should", I am simply saying that Nick's chosen default is consistent with my experiences, and therefore it seems reasonable to me. I'm sure the other people that provided feedback to Nick had slightly different suggestions than mine, but Nick's response to my comment seems to suggest that multiple people are experiencing similar success with a smaller timestep.

@feathern
Copy link
Contributor Author

feathern commented Dec 29, 2021

Indeed, it seems that a few users have observed situations where a timestep somewhat less than 0.1Ek is necessary for stability. Our primary concern at the moment is that this issue has caught several new users by surprise in the past. A PR enabling finer-grained control of this aspect of the CFL will be submitted in the near future. Thank you both for your perspectives. As this PR is now closed, I think it's prudent to close this thread as well and move any subsequent discussion to the CIG forums.
-Nick

@geodynamics geodynamics locked as resolved and limited conversation to collaborators Dec 29, 2021
@feathern feathern deleted the coriolis branch May 10, 2024 15:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants