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

Use kinematic not dynamic viscosity for dt calc #615

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

MTCam
Copy link
Member

@MTCam MTCam commented Feb 25, 2022

Fixes the viscous dt calculation where wrong viscosity was used.

Questions for the review:

  • Is the scope and purpose of the PR clear?
    • The PR should have a description.
    • The PR should have a guide if needed (e.g., an ordering).
  • Is every top-level method and class documented? Are things that should be documented actually so?
  • Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
  • Does the implementation do what the docstring claims?
  • Is everything that is implemented covered by tests?
  • Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

@inducer
Copy link
Contributor

inducer commented Feb 25, 2022

Would it make sense to include a test (i.e. effectively fix, or start to fix, #614) as part of this?

@MTCam
Copy link
Member Author

MTCam commented Feb 25, 2022

Would it make sense to include a test (i.e. effectively fix, or start to fix, #614) as part of this?

Yes, I would like that. Do you have any ideas about how we can effectively test it?

error = (dt_expected - dt_field) / dt_expected
assert actx.to_numpy(discr.norm(error, np.inf)) == 0
error = (dt_field - dt_expected) / dt_expected
assert actx.to_numpy(discr.norm(error, np.inf)) < tol
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this doesn't actually check the stability of the PDE solver at these time steps. Is that something we could incorporate?

Copy link
Member Author

@MTCam MTCam Mar 1, 2022

Choose a reason for hiding this comment

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

That's right, it only checks that we implemented the formula correctly. I think we decided at today's meetings that the validation test(s) for the solver do not need to be integrated and we can add a documentation blurb about (a) reproducing the tests using the provided examples, and (b) how to estimate stability limits for your given problem.

(#614 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

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

(...) Is that something we could incorporate?

I imagine we can test for a contrived case at what CFL it goes unstable (i.e. when infs, or nans appear in the derived quantitites), and make sure that is somewhere between .1 - 1.1 , or even tighter for inviscid problems. This is how I've checked it by-hand.

To do this in the unit testing, we need a NS and Euler test steppers that support constant CFL timestepping. One for Euler is here:

def _euler_flow_stepper(actx, parameters):
,

but I do not believe I have tested it with constant CFL yet. An NS stepper is not something that can go into this PR as NS is downstream of this.

I wonder if it is worth it to test it at this level? Our timestep estimator is designed to return a conservative estimate for the limiting simulation-wide DT, or CFL, so I think as long as it gives a ballpark measure of the {DT, CFL}, i.e. if the simulation is "stable" at CFL < [.1, 1.1] , then it's ok for this purpose.

A useful thing to have would be a routine that returns precisely (up to characterstic_length_scales) the limiting timestep for each of advection, thermal diffusion, species diffusion, and chemistry, at least so they can be visualized. Taking the min DT among these should ensure stability. What we have currently globs them all together to get a conservative value that should ensure stability for the current state.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

2 participants