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.

Sign up for GitHub

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

Jump to bottom

[MPAS-Ocean standalone] Allow land-ice topo to come from MPAS variables #5553

Merged
merged 3 commits into from
Mar 30, 2023
Merged

[MPAS-Ocean standalone] Allow land-ice topo to come from MPAS variables #5553

merged 3 commits into from
Mar 30, 2023

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Mar 24, 2023 •
edited by jonbob
Loading

This applies only to the global_ocean test case in init mode for standalone MPAS-Ocean. E3SM simulations are unaffected.

In the same way that we could previously bypass the interpolation from a lat-lon file for bathymetry, this merge allows us to bypass the interpolation of variables related to land-ice topography and instead read them in from a stream.

[BFB]

@xylar xylar added mpas-ocean BFB PR leaves answers BFB labels Mar 24, 2023
@xylar xylar requested review from sbrus89 and mark-petersen March 24, 2023 11:50
xylar
xylar commented Mar 24, 2023
View reviewed changes
components/mpas-ocean/src/mode_init/mpas_ocn_init_global_ocean.F
Comment on lines +3051 to 3059
if (config_global_ocean_land_ice_topo_file == 'none' .and. &
config_global_ocean_topography_source == 'latlon_file') then
call mpas_log_write( 'Validation failed for global ocean. '// &
'Invalid filename for config_global_ocean_land_ice_topo_file', MPAS_LOG_CRIT)
iErr = 1
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, you had to provide a placeholder name for config_global_ocean_land_ice_topo_file even if you used config_global_ocean_topography_source = 'mpas_variable'. With this change, you should no longer get an error when you don't provide a placeholder filename.

@xylar
Copy link
Contributor Author

xylar commented Mar 24, 2023 •
edited
Loading

Testing

I have run this in conjunction with MPAS-Dev/compass#566. The topography (bathymetry, ice draft, ice thickness and various masks) are remapped outside of MPAS-Ocean init mode using a conservative mapping file. Then, the results are read in using config_global_ocean_topography_source = 'mpas_variable' from a stream.

The pr test suite on Chrysalis with Intel and Intel-MPI passed fine with these changes and is bit-for-bit with 0ee1f84, the base I used for this branch.

@xylar
Copy link
Contributor Author

xylar commented Mar 24, 2023

@sbrus89 and @mark-petersen, I'd appreciate your review on this when you can. It's somewhat urgent because we have a lot of steps to get working before we can make E3SM v3 meshes, and this is early in the chain.

@xylar xylar mentioned this pull request Mar 24, 2023
Merged
6 tasks
@xylar
Copy link
Contributor Author

xylar commented Mar 24, 2023

No my ocean/global_ocean/QU240/PHC/init unexpectedly has non-bit-for-bit results. I need to figure out why. No namelist differences so it must be the code changes.

@xylar xylar force-pushed the mpas-ocean/init-global-ocean-land-ice-topo-from-stream branch from 2151da7 to 572b82d Compare March 27, 2023 13:28
@xylar
Allow land-ice topo to come from MPAS variables
b540d3d 
In the same way that we could previously bypass the interpolation
from a lat-lon file for bathymetry, this merge allows us to
bypass the interpolation of variables related to land-ice
topography and instead read them in from a stream.
@xylar xylar force-pushed the mpas-ocean/init-global-ocean-land-ice-topo-from-stream branch from 572b82d to b540d3d Compare March 27, 2023 13:29
@xylar
Copy link
Contributor Author

xylar commented Mar 27, 2023

@mark-petersen and @sbrus89, This now passes the compass pr suite so it's ready for review.

mark-petersen
mark-petersen approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@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.

Thanks. This looks good. Approving based on visual inspection and the bfb match on the pr test suite, which includes initialization of global meshes.

@xylar
Copy link
Contributor Author

xylar commented Mar 27, 2023

Thanks @mark-petersen!

sbrus89
sbrus89 reviewed Mar 28, 2023
View reviewed changes
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.

@xylar, this is BFB for the tides test case. I just have one minor comment.

components/mpas-ocean/src/mode_init/mpas_ocn_init_global_ocean.F Outdated Show resolved Hide resolved
@xylar
Copy link
Contributor Author

xylar commented Mar 29, 2023

@sbrus89, could you have another look as soon as you're able?

sbrus89
sbrus89 approved these changes Mar 29, 2023
View reviewed changes
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.

@xylar, this looks good to me. Thanks for adding the additional logic. I was also going to suggest the unexpected value error, so I like that change as well.

jonbob added a commit that referenced this pull request Mar 29, 2023
@jonbob
Merge branch 'xylar/mpas-ocean/init-global-ocean-land-ice-topo-from-s…
cea10a0 
…tream' into next (PR #5553)

Allow land-ice topo to come from MPAS variables

This applies only to the global_ocean test case in init mode for
standalone MPAS-Ocean. E3SM simulations are unaffected.

In the same way that we could previously bypass the interpolation from a
lat-lon file for bathymetry, this merge allows us to bypass the
interpolation of variables related to land-ice topography and instead
read them in from a stream.

[BFB]
@jonbob
Copy link
Contributor

jonbob commented Mar 29, 2023

passes sanity testing, merged to next

@jonbob jonbob merged commit bc2f821 into E3SM-Project:master Mar 30, 2023
@jonbob
Copy link
Contributor

jonbob commented Mar 30, 2023

merged to master

@xylar xylar deleted the mpas-ocean/init-global-ocean-land-ice-topo-from-stream branch March 30, 2023 19:03
@xylar
Copy link
Contributor Author

xylar commented Mar 30, 2023

Thanks so much @jonbob, @sbrus89 and @mark-petersen!

xylar added a commit to xylar/compass that referenced this pull request Apr 2, 2023
@xylar
Update E3SM-Project submodule
50032c4 
This merge updates the E3SM-Project submodule from [c9201a4](https://github.com/E3SM-Project/E3SM/tree/c9201a4f44540bb74cb3650e32bcbe27fb762ab1) to [b4d5b10600](https://github.com/E3SM-Project/E3SM/tree/b4d5b10600).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#5254
- [ ]  (fwk) E3SM-Project/E3SM#5490
- [ ]  (ocn) E3SM-Project/E3SM#5541
- [ ]  (fwk) E3SM-Project/E3SM#5498
- [ ]  (ocn) E3SM-Project/E3SM#5564
- [ ]  (ocn) E3SM-Project/E3SM#5553
- [ ]  (ocn) E3SM-Project/E3SM#5519
This was referenced Apr 2, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mark-petersen mark-petersen mark-petersen approved these changes

@sbrus89 sbrus89 sbrus89 approved these changes

Assignees

@jonbob jonbob

Labels
BFB PR leaves answers BFB mpas-ocean
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xylar @jonbob @sbrus89 @mark-petersen