Repository navigation
Compute MeshScaling for variable-resolution meshes - #506
Conversation
| TracerVertMixTendencyEnable: true | ||
| HorzMesh: | ||
| MeshScaling: | ||
| ScaleWithMesh: false |
There was a problem hiding this comment.
I personally think we should get rid of ScaleWithMesh entirely and not have any support for MeshDensity. The tools for mesh creation now just fill it with all ones. so it is not going to be useful in new meshes. The term ScaleWithMesh is super confusing because it seems like it means UseRefWidth.
There was a problem hiding this comment.
I thought this is what we already agreed to in #476 (comment)
There was a problem hiding this comment.
@xylar , thanks. Right. I kept that part of the code just in case, mainly for comparison with MPAS-Ocean. Since UseRefWidth: true is 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.
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.
| MeshScaling: | ||
| ScaleWithMesh: false | ||
| UseRefWidth: true | ||
| RefWidth: 30.0e3 |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
@cbegeman and @xylar , thanks for your thoughts on this. I agree. I also had some bad experiences with these settings.
To summarize next steps,
- Removal of
MeshScalingsetting inomega.yml - Removal of
MeshDensityrelatedMeshScalingcomputations - Compute the global median cell width (
$\Delta x_{med}$ ) at initialization - Compute
MeshScalingDel2andMeshScalingDel4from$\Delta x_{med}$ :
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 IcoswISC30E3r5 and EC30to60E2r2. This suggests that we may need to retune the Del2 and Del4 viscosity coefficients for Omega for each different mesh? Please let me know if I am misunderstanding anything.
There was a problem hiding this comment.
One concern I have is that
$\Delta x_{med}$ on a variable-resolution mesh may be larger than the minimum grid spacing, or its theoretical value.
@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 IcoswISC30E3r5 and EC30to60E2r2, though they wouldn't differ by too much. I helpful piece of information for debugging would be to have Omega output:
$\Delta x_{med}$ - the min and max values of the del2 and del4 coefficients.
There was a problem hiding this comment.
Do you agree with this plan?
Yes, I agree.
52f4931 to
dbe3b6f
Compare
|
@hyungyukang Great to see you're getting back to this. I had missed your prior question until now but happy to take a look again when you like or when it's ready for review. I accidentally hit the close button -- sorry about that! |
dbe3b6f to
2cd0411
Compare
|
@hyungyukang and @cbegeman, this fix is still badly needed and I think the revisions discussed in #506 (comment) still aren't here (even after the rebase). This is causing difficulties in E3SM-Project/polaris#643 becasue without it the viscosity for non-uniform meshes is messy. It is also badly needed for the unified mesh. Could we revive this as a priority? |
|
@xylar , Sorry for the delay. I was working on this, but I hadn’t found a good time to test it. I’m making it my highest priority now. |
- Add mesh-density-based scaling for del2 and del4 viscosity/diffusion - Follow the MPAS-Ocean approach for computing mesh scaling
- Replace mesh scaling options with MeshScalingEnable - Drop ScaleWithMesh, UseRefWidth, MaxMeshDensity and the MeshDensity- based scaling. - A single HorzMesh MeshScalingEnable switch (default true) scales the mixing coefficients with the cell width relative to the global median cell width; when false, both coefficients are one.
aecc383 to
f8a3562
Compare
|
@xylar and @cbegeman , I’ve addressed the comments and suggestions. The mesh scaling is now computed based on the global median cell width of the given variable-resolution mesh. I ran one-day simulations on the I think it’s ready for another review. Could you take a look when you have time? |
xylar
left a comment
xylar
left a comment
There was a problem hiding this comment.
@hyungyukang, thanks, this now does what we agreed in #506 (comment): the MeshDensity path and the nested options are gone, the median is exact and independent of the decomposition, and the log reports the median and the range of both factors. The conversion table in the User's Guide is helpful, and your EC30to60E2r2 median (35.92 km) matches what I measured from that mesh (35.9 km).
One thing is left, inline: the default.
Drafted by @xylar and Claude Code together
| KPPNonLocalTracerFluxTendencyEnable: true | ||
| KPPNonLocalTracerDiagnosticsEnable: false | ||
| HorzMesh: | ||
| MeshScalingEnable: false |
There was a problem hiding this comment.
The last commit switched this to false, but the User's Guide still says scaling is on by default and shows true. The new test in HorzMeshTest.cpp also checks the scaled coefficients the mesh was created with, so I'd expect it to fail with the default off on any mesh whose cells aren't all the same area. We agreed scaling should be the default, so I'd set this back to true. That will change omega_pr results on meshes with unequal cell areas, which the Testing comment can note.
There was a problem hiding this comment.
Thanks. I was thinking to match MPAS-O's default. I switched back to true.
|
Some things will need to change on the Polaris side so we scale things correctly for Omega and MPAS-Ocean based on this update. I'll work on that and run the |
Enabled `MeshScalingEnable` by default in `Default.yml`
Thanks, @xylar, for the quick review. I think the realistic global ocean test cases on variable-resolution meshes in Polaris need to be updated. Please let me know if there’s anything I can help with. |
Polaris
|
Testing on aurora: CTests onlyI reran only the CTests on Aurora with Intel and IntelGPU (MPICH), for the merge of #506 ( CTest unit tests:
CTest unit tests:
Posted by Claude Code on @xylar's behalf. The testing, analysis and wording above are AI-authored; please check them accordingly. |
Testing on frontier: CTests onlyI reran only the CTests on Frontier with craygnu and craygnu-mphipcc (MPICH), for the merge of #506 ( CTest unit tests:
CTest unit tests:
Posted by Claude Code on @xylar's behalf. The testing, analysis and wording above are AI-authored; please check them accordingly. |
xylar
left a comment
xylar
left a comment
There was a problem hiding this comment.
I like @cbegeman's suggestion but I'd like to get this in before #598.
I tested this in Polaris in E3SM-Project/polaris#867 including the two realistic global tests with cached meshes. This is doing what we want and is comparable to the MPAS-Ocean equivalent when we account for the different reference cell size.
|
@cbegeman, it wasn't clear if you want to see testing beyond what I did in E3SM-Project/polaris#867 before we merge this. Please let us know. |
I wasn't aware that any non-B4B PR's were merged to develop, although it's possible something slipped past with NERSC, LCRC, and ALCF being down over the past week. |
|
@sbrus89, you're right; that sentence was wrong. Nothing answer-changing has merged into The results above stand; only the stated reason for the baseline was wrong. I've struck it out in the four testing comments. Posted by Claude Code on @xylar's behalf. The testing, analysis and wording |
|
@sbrus89, I'm making changes to the tool to hopefully prevent this kind of nonsense from creeping in again. The agent said it guessed based on the PR title that it might be non-BFB and made some other pretty confusing excuses. I'm making the instructions quite a bit stricter. Thanks for pointing out the mistake! |
Can you elaborate on what your testing in E3SM-Project#867 shows? It's not immediately apparent to me from the figures how the effects of mesh scaling are manifesting in EC30to60. The comment could be on that PR. |
|
That's fair. I'll produce more relevant plots in E3SM-Project/polaris#867 and post there. |
|
@cbegeman, the plots are in E3SM-Project/polaris#867 (comment). With MPAS-Ocean's reference width set to Omega's median, the two models' ν2 and ν4 are bit-for-bit identical on every ocean edge of Posted by Claude Code on @xylar's behalf. The testing, analysis and wording above are AI-authored; please check them accordingly. |
|
@xylar Thank you so much for that additional analysis and testing! That more than shows that this PR is working as expected. |
cbegeman
left a comment
cbegeman
left a comment
There was a problem hiding this comment.
Thank you so much for tackling this @hyungyukang ! It's clear from @xylar 's testing that this is functioning as intended.
Add viscosity coefficients to config_overrides.yaml used for coupled mode.
Testing on chrysalisTested the merge of #506 (
chrysalis, intel, openmpiPolaris
|
Testing on auroraTested the merge of #506 (
aurora, intel, mpichPolaris
|
Testing on pm-cpu and pm-gpuTested the merge of #506 (
pm-cpu, gnu, mpichPolaris
|
|
I agree that E3SM-Project/polaris#860 and maybe E3SM-Project/polaris#857 explain the diffs. |
Testing on frontierTested the merge of #506 (
frontier, craygnu, mpichPolaris
|
There was a problem hiding this comment.
@hyungyukang Couple of minor comments about unneeded entries, since they match the defaults in config/Default.yml.
But otherwise looks great!
|
The diffs in my testing can be ignored, the utility didn't do a local merge of E3SM-Project/polaris#867 into |
Address PR reviewer's suggestion Co-authored-by: Andrew Nolan <32367657+andrewdnolan@users.noreply.github.com>
Address PR reviewer's suggestion Co-authored-by: Andrew Nolan <32367657+andrewdnolan@users.noreply.github.com>
|
@sbrus89 I'd say unit test and validation that run as part of CI should be sufficient for testing those config changes. Looks like those are all passing! |
|
Thanks @sbrus89 ! |
This brings in E3SM-Project/Omega#506, which adds mesh scaling of the horizontal mixing coefficients. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This PR adds spatially variable
MeshScalingDel2andMeshScalingDel4inHorzMesh.cppso that horizontal viscosity and diffusivity scale with the local mesh resolution, following Eqs. (1) and (2) of Hoch et al., JAMES 2020. The reference width is the global median cell width rather than a user-specified width, as discussed in #506 (comment).This PR resolves #476.
Changes
HorzMesh::computeMedianCellWidth(), which computes the global median cell width at initialization. Each cell's width is the diameter of a circle with the same area,MeshScalingDel2MeshScalingDel4config/Default.yml, disabled by default. Setting it tofalsegives coefficients of one everywhere.Checklist
Documentation:
Linting
Building
Testing
aurora, intel, mpich
aurora, intelgpu, mpich
chrysalis, intel, openmpi
frontier, craygnu, mpich
frontier, craygnu-mphipcc, mpich
pm-cpu, gnu, mpich
pm-gpu, gnugpu, mpich
Provide relevant details in a comment to the PR titled
Testingwith the following:have been run on and indicate that are all passing.
has passed, using the Polaris
e3sm_submodules/Omegabaseline-pfor both the baseline (Polarise3sm_submodules/Omega) and the PR build