-
Notifications
You must be signed in to change notification settings - Fork 8
Compute MeshScaling for variable-resolution meshes
#506
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
base: develop
Are you sure you want to change the base?
Changes from all commits
6b72c7d
e4f9ff4
268f173
54b0b66
52f4931
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,12 @@ Omega: | |
| PressureGradTendencyEnable: true | ||
| VelVertMixTendencyEnable: true | ||
| TracerVertMixTendencyEnable: true | ||
| HorzMesh: | ||
| MeshScaling: | ||
| ScaleWithMesh: false | ||
| UseRefWidth: true | ||
| RefWidth: 30.0e3 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that this is a good idea because it would be easy for users (or buildnml) to neglect to change this option and thus end up applying a different diffusivity than intended. Could this maybe be computed online (at init) as the median cell width? It's not clear to me that this needs to be user-configurable since the user can just change the diffusivities. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also agree. The MPAS-Ocean equivalent settings have gotten me in trouble so many times. I like the median resolution idea. I think MPAS-Ocean has a setting where it uses the minimum but that's a noisy value. The median would be a lot more robust -- just requiring a global reduction at init.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cbegeman and @xylar , thanks for your thoughts on this. I agree. I also had some bad experiences with these settings. To summarize next steps,
Do you agree with this plan? As far as I understand, in MPAS-Ocean, the Del2 and Del4 viscosity coefficients for each horizontal resolution are tuned based on the minimum grid spacing. One concern I have is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@hyungyukang, yes, that would be true by definition, so the coefficient would need to be adjusted accordingly. But the median resolution should be much more robust across different meshes that are generated at ostensibly the same resolution (because of noise in the Jigsaw generation process). The expectation should not be that the same coefficient is right for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. |
||
| MaxMeshDensity: -1.0 | ||
| ManufacturedSolution: | ||
| WavelengthX: 5.0e6 | ||
| WavelengthY: 4.33013e6 | ||
|
|
||
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.
I personally think we should get rid of
ScaleWithMeshentirely and not have any support forMeshDensity. The tools for mesh creation now just fill it with all ones. so it is not going to be useful in new meshes. The termScaleWithMeshis super confusing because it seems like it meansUseRefWidth.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.
I thought this is what we already agreed to in #476 (comment)
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.
@xylar , thanks. Right. I kept that part of the code just in case, mainly for comparison with MPAS-Ocean. Since
UseRefWidth: trueis the default, that approach will not be used. However, I agree that we should remove it to avoid any confusions and potential issues. Will work on 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.
I think we can keep it on a testing branch or something for comparison to MPAS-Ocean but I don't want it to become part of the Omega code unless it's buried enough that no user will try to use it.