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

Extend manufactured solution to del2 and del4 #234

Merged
merged 15 commits into from
Jan 10, 2025

Conversation

cbegeman
Copy link
Collaborator

@cbegeman cbegeman commented Oct 1, 2024

This PR adds manufactured solution tests for del2 and del4 and renames the existing case to default. Both del2 and del4 tests depend on the corresponding source terms being added to the model. These terms are added to MPAS-Ocean here E3SM-Ocean-Discussion/E3SM#110. All tests depend on E3SM-Ocean-Discussion/E3SM#55 to achieve expected convergence.

Checklist

  • User's Guide has been updated
  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes
  • New tests have been added to a test suite

@cbegeman cbegeman added ocean Related to ocean tests or analysis E3SM PR required The polaris changes won't work with the current E3SM-Project submodule and require an update Omega PR required The polaris changes won't work with the current Omega submodule and require an update labels Oct 1, 2024
@cbegeman cbegeman self-assigned this Oct 1, 2024
@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 1, 2024

Testing

MPAS-Ocean tests have been run with E3SM-Ocean-Discussion/E3SM#110 and E3SM-Ocean-Discussion/E3SM#55.

Update 19 Dec: MPAS-Ocean has been re-run and tests pass

@cbegeman
Copy link
Collaborator Author

@xylar and @sbrus89 Would you like to look this over again and get this in before the IO streams changes? I think I'd rather do those changes to this test separately.

@xylar
Copy link
Collaborator

xylar commented Dec 19, 2024

I'm not working much tomorrow but I can try to test this out. I agree that it would be good to get it in first without worrying about Omega, and then rework it for Omega support.

@xylar xylar removed the Omega PR required The polaris changes won't work with the current Omega submodule and require an update label Dec 23, 2024
@xylar
Copy link
Collaborator

xylar commented Dec 23, 2024

Just a note that this needs E3SM-Project/E3SM#6862 and and update to the E3SM-Project submodule.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

I only have one other concern besides the Omega support and that is that the analysis plots for the 3 tests look exactly identical, which I wouldn't have expected.

convergence_ssh_default
convergence_ssh_del2
convergence_ssh_del4

Does that suggest that the del2 and del4 terms are negligible and we need to tweak the parameters?

Comment on lines 36 to 38
Tendencies:
VelDiffTendencyEnable: false
VelHyperDiffTendencyEnable: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cbegeman, this change will break manufactured solution in Omega because the associated changes in forward.py don't yet include anything related to Omega. I know you don't want to address Omega here yet but maybe the solution is: 1) to leave these changes out for Omega and 2) to have setup fail for Omega for the del2 and del4 tasks for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xylar I removed the whole Omega section from this file. Is that in line with what you were thinking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cbegeman, hmm, I'm hesitant to drop Omega support for manufactured_solution entirely. It's one of only 2 working tests we have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xylar Should I just go ahead and make the io streams changes then to support all 3?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xylar I reverted the changes to this file. Should we merge this PR and then start adding support for Omega del2/del4?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, thanks! Yes, I think so. (Sorry, I somehow missed the earlier comment).

I think we need to rebase or otherwise resolve the conflict listed below before we can merge.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Jan 2, 2025

I only have one other concern besides the Omega support and that is that the analysis plots for the 3 tests look exactly identical, which I wouldn't have expected.

The convergence rates are different for each test. Is it important that the analysis plots look different?

@xylar
Copy link
Collaborator

xylar commented Jan 2, 2025

The convergence rates are different for each test. Is it important that the analysis plots look different?

Hmm, let me check that that's true for my test. The plots are visually identical so I assumed the convergence rates would be, too. It seems like if the del2 and del4 terms are actually having an effect, the error would be more quantitatively different with and without the terms, wouldn't you think?

@cbegeman
Copy link
Collaborator Author

cbegeman commented Jan 2, 2025

@xylar My memory was foggy. I should have said that the errors are different for each test. I didn't check the convergence rates to enough significant digits to tell.

@xylar
Copy link
Collaborator

xylar commented Jan 2, 2025

cat */analysis/*.csv
default:
resolution,l2
200.0,0.31642597886910423
100.0,0.07877610404218326
50.0,0.019694390950707373
25.0,0.004945690456387108

del2:
resolution,l2
200.0,0.3164142762184392
100.0,0.0787731760256298
50.0,0.019693657273716133
25.0,0.00494550529879111

del4:
resolution,l2
200.0,0.316425984359642
100.0,0.07877611000352792
50.0,0.01969439697649461
25.0,0.004945696294213287

It seems like differences are in something like the 5th digit. To me, that suggests that the del2 and del4 terms aren't big enough to be affecting the convergence rate. We wouldn't notice if those terms don't have the expected convergence rate because all three tests are really only showing the convergence rate of the dominant terms.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Jan 2, 2025

@xylar That's a good point. We'd want that term to be large enough that you'd notice if you broke the del2/del4 operators. I'll test some other values shortly.

@xylar
Copy link
Collaborator

xylar commented Jan 2, 2025

Thanks, that sounds good!

@cbegeman
Copy link
Collaborator Author

cbegeman commented Jan 6, 2025

With these changes the convergence rates are:
Default: 2.044
Del2: 2.042
Del4: 2.039

I already had to decrease the time step by a factor of 2 to accommodate these higher viscosities. If we want to increase the del2 viscosity further, we will need to further decrease the time step. I would rather not do this because it's nice to have this be a quick (1 min) test.

@xylar
Copy link
Collaborator

xylar commented Jan 6, 2025

@cbegeman, okay, that seems like a reasonable compromise. It seems like we're still going to have a hard time telling if we've messed up the either the del2 or del4 term using these tests but maybe that just suggests we would need a dedicated tests to determine that if it were a high priority for us. We can discuss more outside of this PR if need be.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Approving based on my previous testing and @cbegeman's newest testing.

@xylar
Copy link
Collaborator

xylar commented Jan 6, 2025

In case my comment above gets lost, I think the remaining step is to rebase or merge main into this branch to address the conflict listed below.

@cbegeman cbegeman force-pushed the enhance-manufactured-solution branch from 5d2eea9 to 071f244 Compare January 6, 2025 22:21
Copy link
Contributor

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

@cbegeman, this is great! I tested on Perlmutter and got the same convergence results you did above. My one comment is that although the errors are clearly converging, the viz step seems to be showing incorrect differences in the left panel below:

image

@cbegeman
Copy link
Collaborator Author

cbegeman commented Jan 8, 2025

@sbrus89 Thanks for catching that! I've added a fix here: d48ea7b
image

Copy link
Contributor

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the viz step, @cbegeman. The changes look good.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Jan 9, 2025

@xylar Can you check that I've done the submodule update correctly before I merge? It's been a while since I've done one of those.

@xylar
Copy link
Collaborator

xylar commented Jan 10, 2025

@cbegeman, yes, looks great to me!

@xylar xylar assigned xylar and unassigned cbegeman Jan 10, 2025
@xylar xylar added E3SM PR finished The polaris changes required an update to the E3SM-Project submodule and this is now finished and removed E3SM PR required The polaris changes won't work with the current E3SM-Project submodule and require an update labels Jan 10, 2025
@xylar xylar merged commit 10029fa into E3SM-Project:main Jan 10, 2025
5 checks passed
@xylar xylar deleted the enhance-manufactured-solution branch January 10, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E3SM PR finished The polaris changes required an update to the E3SM-Project submodule and this is now finished ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants