-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Updating Limiters for Finite Volume Formulation #28892
base: next
Are you sure you want to change the base?
Conversation
@freiler plase also take a look if you can |
To-do:
|
you'll want to get rid of the submodule update in the wasp folder |
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.
Should I wait to review this until you've removed the WIP label? In which case should we convert this to draft?
You can mention me once you're done pushing and I will review. Going to unsubscribe for now |
We'll want to reduce the size of the tests. 25x25 for a full grid of tests is bloating the repo. You ll see it fail the same way with 5x5 but at (theoretically) 1/25th of the memory cost |
Job Documentation, step Docs: sync website on 2c4b562 wanted to post the following: View the site here This comment will be updated on new commits. |
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.
early feedback, not quite done but might be delayed tomorrow
[] | ||
[] | ||
[top_right_limited_scalar_advection] | ||
requirement = 'The system shall be able to perform a variety of limiting schemes when solving scalar transport equations in cartesian meshes with top-right advection. These schemes include' |
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.
are we expecting any additional insights from top-right as opposed to bottom-left?
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.
Yes. The gradient limiting is directional. I just want to make sure that no direction is ever messed up
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.
The gradient limiting is directional as in it depends on which direction is upwind. high/low X, Y, Z are not coded anywhere?
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.
No. It also has a directional component when computing the gradient at the center of the cell. If someone messess this up it needs to show in these tests
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.
@grmnptr do you understand why that would be?
I m at a loss on this.
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.
how is top right any different from bottom left in the code?
we dont hard code any direction as different from any other?
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.
The way in which you do second order limiting, goes as follows:
$$\phi_f = \phi_c + (\nabla \phi)c d{fc}$$
(\nabla \phi)c is computed per direction, e.g., in 2D, (\nabla \phi)c = [(\nabla \phi){c,x}, (\nabla \phi){c,y}]
Note that in top-left advection, each component of the limiting gradient are positive, whereas in bottom-right, they are negative. If someone ever messes up the directionality of this gradient (e.g., by limiting to zero the minimum components of the gradient), it needs to show up in the failing tests but it won't if we are only doing advection from the bottom-left
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.
ok I see now what you are thinking of.
Unfortunately we cant be targeting each potential mistake in modifications using regression tests. For example, what if someone switches the Y and X components? Both of these 45 degree tests would not catch it, yet we wont add 5 more tests to make sure this does not happen.
Testing the limiter for such behavior should be done in a unit test if you care to prevent it.
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. All limiters don't work the same and depend on the gradient somehow. So, I will still need to test them in both directions. I can remove the upwind test for advection from the top-right if you like. It is the only redundant one that I can think of
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.
if you use the 'hide' parameter as shown in the latest comments I expect the file size will go down enough that it does not matter as much that the tests are doubled
9baadd6
to
c6913a1
Compare
Job Precheck, step Clang format on c6913a1 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
1 similar comment
… vanLeer vector limiter (ref difs ~1e-5) Refs idaholab#28891
…nvergence is at leaste the required order of convergence Refs idaholab#28891
…ng test tolerance Refs idaholab#28891
Co-authored-by: Guillaume Giudicelli <[email protected]> regolding tests to latest mods in limiters and avoid testing unnecessary recovers Refs idaholab#28891
Co-authored-by: Peter German <[email protected]>
I dropped the submodule upodate and fixed (we will see based on the tests) some tests. |
I also did the technical review on this PR yesterday. As part of that, I ran the tests manually and noticed a few things:
|
I remembered wrong, considering that I did not run bigger, more application-oriented problems, my review is not a technical review. Just observations based on the test cases. |
Closes #28891