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

Update namelist options for recent changes to Redi and Adams-Bashforth 2 #745

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Dec 3, 2023

This merge sets config_Redi_min_layers_diag_terms = 0, consistent with E3SM-Project/E3SM#6077.

It also changes the time integrator from split_explicit to split_explicit_ab2 nearly everywhere.

Checklist

  • Document (in a comment titled Testing in this PR) any testing that was used to verify the changes

@xylar xylar added enhancement New feature or request ocean E3SM PR finished labels Dec 3, 2023
@xylar xylar requested a review from mark-petersen December 3, 2023 18:25
@xylar xylar self-assigned this Dec 3, 2023
@xylar xylar requested a review from cbegeman December 3, 2023 18:26
@xylar
Copy link
Collaborator Author

xylar commented Dec 3, 2023

@mark-petersen and @cbegeman, I will test this to make sure all affected tests still work.

Once that's done, I'd appreciate your feedback to make sure we don't want to keep split_explicit (as opposed to split_explicit_ab2) for any tests.

config_time_integrator = 'split_explicit'
config_time_integrator = 'split_explicit_ab2'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cbegeman, would it be better to stick with split_explicit for isomip_plus? (Should we explicitly do this for ice_shelf_2d as well?) I think this might be necessary for wetting and drying work you're doing, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@xylar This change is fine. When we are happy with split_explicit wetting and drying, I will change config_time_integrator='RK4' to config_time_integrator='split_explicit' in namelist.thin_film.forward_and_ssh_adjust. Let's not worry about ice_shelf_2d. I was only planning on making enhancements (incl. adding additional tests) in polaris and not migrating those changes back to compass.

xylar added 2 commits December 3, 2023 21:31
Only the nonhydro test case is still split_explicit (for now).
@xylar xylar force-pushed the update-redi-and-ab2 branch from 6b37529 to 764ff7a Compare December 4, 2023 04:31
@xylar
Copy link
Collaborator Author

xylar commented Dec 4, 2023

Testing

I ran the pr suite, the gotm test, the two internal_waves default tests, the isomip_plus z-star Ocean0 test, and the 4 sphere_transport tests on Chrysalis with Intel and OpenMPI. All tests ran successfully. I haven't looked at the output carefully or compared with baselines because the changes are expected to be larger than machine precision ("climate changing").

@xylar
Copy link
Collaborator Author

xylar commented Dec 4, 2023

@mark-petersen and @cbegeman, this is ready to review. It's not a high priority but it would be nice to be testing as consistently with E3SM itself as we can be.

@cbegeman
Copy link
Collaborator

cbegeman commented Dec 5, 2023

@mark-petersen Could you weigh in on whether this internal_wave test result is satisfactory? I wouldn't have expected there to be negative values.

AB2
image

split_explicit
image

@mark-petersen
Copy link
Collaborator

@xylar thanks for changing these tests. I agree that we should change to AB2 in all compass tests, since we made the switch in E3SM.

@cbegeman thanks for noticing the negative relative RPE in the internal wave test case. Note that the vertical scale is a factor of 10 smaller, which means less spurious mixing over time. The negative values are concerning. It means that "anti-mixing" is going on, which would be gradient steepening or grid-scale noise. We should view some horizontal slices of temperature at those negative RPE points and compare the two time step schemes, to see what is going on.

It might be that the timestep needs to be shortened a bit for AB2 relative to split explicit, because there are fewer iterations within each timestep, and we are seeing the result of a numerical instability.

@cbegeman
Copy link
Collaborator

cbegeman commented Dec 6, 2023

@mark-petersen Do you have time to explore the time step sensitivity? If we want to get this PR in soon, we could remove this change from all of the internal_wave tests.

@xylar
Copy link
Collaborator Author

xylar commented Jan 2, 2024

@mark-petersen, I think we're still waiting for your response here.

@mark-petersen
Copy link
Collaborator

Sorry I missed this. I'm out for a week, so I think it would be better to remove the flag change from the internal_wave test case.

Copy link
Collaborator

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

Please merge after removing changes from internal_wave tests. The AB2 was tested at length for global simulations.

@xylar
Copy link
Collaborator Author

xylar commented Jan 9, 2024

@cbegeman, I switched the internal waves test back to split_explicit. Would you be in a good position to test it again? If things work out, I think we can hopefully get this branch merged.

@cbegeman
Copy link
Collaborator

cbegeman commented Jan 9, 2024

@xylar and @mark-petersen I don't feel the need to test again now that the internal_wave test case changes are removed. The other tests checked out fine when I ran them earlier. If you would really like me to retest I can.

@xylar
Copy link
Collaborator Author

xylar commented Jan 9, 2024

@cbegeman, sure that's fine.

@xylar
Copy link
Collaborator Author

xylar commented Jan 9, 2024

Thanks for the earlier testing and review!

@xylar xylar merged commit a2a9579 into MPAS-Dev:main Jan 9, 2024
4 checks passed
@xylar xylar deleted the update-redi-and-ab2 branch January 9, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants