Integration of mom6_forge & regional_mom6 (Remerged)

[manishvenu]

Co-authored-by: @ashjbarnes

Closes CROCODILE-CESM/CrocoDash#183

Summary:

Integrate mom6_bathy into regional_mom6. There are NO front-end changes to this PR (the one exception is slight changes to what is printed out in the setup_bathymetry function).

Shift the grid functions from regional_mom6 to mom6_bathy to eventually take advantage of the functions in mom6_bathy. For now, this PR just reproduces the user facing functions.

This PR will add the mom6_bathy as a dependency (mom6_bathy is soon to be renamed and released on pypi).

Changes:
expt.make_hgrid, make_vgrid, setup_bathymetry now use mom6_bathy grid, topo, and vgrid

Advantages:

There are a ton of advantages, but some highlights:

Gain access to useful functions in mom6_bathy:

1. Runoff Mapping
2. Chlorophyll File Generation for regional domains
3. Generate ESMF Mesh files (for NOUPC Coupling) and CICE grids with ease from a bathmetry file
4. Use the Topo Editor (an interactive widget for bathymetry editing)
5. EDIT: My favorite advantage is using the mom6_bathy fill method! Here is an example of how nice this fill method is when you use a low resolution product (like global CESM output).

Regional_mom6/CROCODILE Advantages:
7. Merge the grid generation advances CROCODILE & regional_mom6 make together
8. mom6_bathy gains additional functions in generating hgrids and topos
9. More easily understand what a grid, topo, and vgrid are inside of regional_mom6-> they're objects instead of netcdf files
10. This will go a huge way to making these files more self-describing!

Disadvantages/Considerations:

1. Add another layer of complexity to regional_mom6
2. Currently, this development is on a branch of NCAR/mom6_bathy, which can make editing challenging. (I'd totally recommend COSIMA make a fork)
3. The big kicker is that this moves all of the grid generation out of regional_mom6 into mom6_bathy (Since mom6_bathy will be installed as a pip dependency, edits to grid generation would be PRs into the mom6_bathy repo)

Companion PR: https://github.com/NCAR/mom6_bathy/pull/34

For reviewers/maintainers:

1. Feature requests: This PR is ONLY to refactor where code is. Any major feature requests should go into issues! If you're reviewing this PR and notice something cool you want that's going to take a while to develop, make an issue. However, if it's small, I'm more than happy to do it.
2. BASELINES!: As briefly discussed a month or so ago, there's a definite need for baselines for both CrocoDash and regional_mom6. I'm thinking this (kind of major) PR is a great place to start doing that for the grids aspect. Here is the way I'm doing them for now. If we're interested in starting them up on the COSIMA computer as well (which we definitely should!), please feel free to copy this code and try it out. I run it with:

python baseline_grid_generation.py <baseline_dir> --with-bathy # If you add the --with-bathy flag make sure you request additional resources

3. How to test this PR! You need to install mom6_bathy! Which can be done by cloning it than pip install -e . in the cloned folder

4. Instawins: Since a lot of these advantages will take some time & demo-writing, I dropped a demo for the TopoEditor, ESMF, & CICE files in this PR that should work, but will hold off on other demos for further PRs.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ashjbarnes]

On testing, one thing I noticed is that regridding the bathymetry outputs this message:

Setting up bathymetry...if this fails, please follow the printed instructions with your experiment topo object, like this: [experiment_obj].topo 
**NOTE**
            If bathymetry setup fails (e.g. kernel crashes), restart the kernel and edit this cell.
            Call ``[topo_object_name].mpi_set_from_dataset()`` instead. Follow the given instructions for using mpi 
            and ESMF_Regrid outside of a python environment. This breaks up the process, so be sure to call
            ``[topo_object_name].tidy_dataset() after regridding with mpi.
Begin regridding dataset...

It works fine, but the printed message is confusing, because the "given instructions for using mpi and esmf_regrid" don't seem to print out any more. These instructions used to print at the top of the cell.

I'll go find where this should be printed and see if it's a matter of changing its log level

[ashjbarnes]

I also get this MOM error, which suggests that this refactor has changed the units of vcoord:

FATAL from PE 0: MOM_ALE, initialize_regridding: Unsupported format in grid definition './INPUT/vcoord.nc'. Error message Units incorrect: meter /= meters

Do the CROCODILE runs use 'meter' rather than 'meters' as units? I can't actually see where this is specified in our MOM_input files, so am puzzled why ALE is looking for a specific unit in the first place.

edit: mom6 seems to have 'meters' hardcoded. Does crocodile override this?

edit: mom6_bathy specifies meter as units here

[manishvenu]

On testing, one thing I noticed is that regridding the bathymetry outputs this message:

Setting up bathymetry...if this fails, please follow the printed instructions with your experiment topo object, like this: [experiment_obj].topo 

**NOTE**

            If bathymetry setup fails (e.g. kernel crashes), restart the kernel and edit this cell.

            Call ``[topo_object_name].mpi_set_from_dataset()`` instead. Follow the given instructions for using mpi 

            and ESMF_Regrid outside of a python environment. This breaks up the process, so be sure to call

            ``[topo_object_name].tidy_dataset() after regridding with mpi.

Begin regridding dataset...

It works fine, but the printed message is confusing, because the "given instructions for using mpi and esmf_regrid" don't seem to print out any more. These instructions used to print at the top of the cell.

I'll go find where this should be printed and see if it's a matter of changing its log level

Huh, it should just be a print statement in mpi_set_from_datasaet in mom6_bathy/topo.py. You do have to run that instead of the original set_from_dataset that's called from setup_bathyemtry.. This print statement needs to be updated for setup_bathymetry. Maybe a flag that calls the mpi_setup?

[manishvenu]

I also get this MOM error, which suggests that this refactor has changed the units of vcoord:

FATAL from PE 0: MOM_ALE, initialize_regridding: Unsupported format in grid definition './INPUT/vcoord.nc'. Error message Units incorrect: meter /= meters

Do the CROCODILE runs use 'meter' rather than 'meters' as units? I can't actually see where this is specified in our MOM_input files, so am puzzled why ALE is looking for a specific unit in the first place.

edit: mom6 seems to have 'meters' hardcoded. Does crocodile override this?

Shoot, this is most likely a bug. Crocodile using the Vgrid.write() method which doesn't use thickness. Rm6 uses thickness so it calls a different vgrid method called write_z_file() that must have a dispelling. I'll make a pr fixing the mistake tomorrow, and we can use that to review this PR?

Basically instead of using the git install in the pyproject, clone mom6_bathy to this PR, then pip install -e, in that folder.

[ashjbarnes]

Shoot, this is most likely a bug. Crocodile using the Vgrid.write() method which doesn't use thickness. Rm6 uses thickness so it calls a different vgrid method called write_z_file() that must have a dispelling. I'll make a pr fixing the mistake tomorrow, and we can use that to review this PR?

Thanks @manishvenu ! Since these bugs are COSIMA specific, how about I make a draft PR that fixes this and any other issues that I find in using mom6_bathy in our workflows. We can work on PR together ofc but you shouldn't have to do it all!

Also: looks like all of the units in the vcoord class, including .write have units of meter. This suggests you guys have this too for your vcoord but for some reason mom6 isn't complaining about it?

[ashjbarnes]

Huh, it should just be a print statement in mpi_set_from_datasaet in mom6_bathy/topo.py. You do have to run that instead of the original set_from_dataset that's called from setup_bathyemtry.. This print statement needs to be updated for setup_bathymetry. Maybe a flag that calls the mpi_setup?

Hey no this is my bad I misunderstood before! No issues, ignore me

[ashjbarnes]

Weird bug I found:

When using the pint convert (already merged) at the boundaries, the manual K->C conversion happens first. Later, the old units are inherited again when attempting the pint convert, so if the manual conversion succeeded, then you do the K->C conversion twice

Weirdly, this only happens on this branch, not on the current main. So presumably the manual conversion was like accidentally failing before, so the bug accidentally didn't happen

I'm just making sure that the manual convert happens after now which should fix it

[ashjbarnes]

This fix has worked, but due to a change in NaN masking value (I think) somewhere in this PR, we now fill the land areas with -273. I'll work on this tomorrow

[manishvenu]

This fix has worked, but due to a change in NaN masking value (I think) somewhere in this PR, we now fill the land areas with -273. I'll work on this tomorrow

If ya want me to take a look at this one, happy to help

[manishvenu]

Weird bug I found:

When using the pint convert (already merged) at the boundaries, the manual K->C conversion happens first. Later, the old units are inherited again when attempting the pint convert, so if the manual conversion succeeded, then you do the K->C conversion twice

Weirdly, this only happens on this branch, not on the current main. So presumably the manual conversion was like accidentally failing before, so the bug accidentally didn't happen

I'm just making sure that the manual convert happens after now which should fix it

Does the output print out that the conversion was done twice? Might be nice to have some kind of warning for this

[manishvenu]

Huh, it should just be a print statement in mpi_set_from_datasaet in mom6_bathy/topo.py. You do have to run that instead of the original set_from_dataset that's called from setup_bathyemtry.. This print statement needs to be updated for setup_bathymetry. Maybe a flag that calls the mpi_setup?

Hey no this is my bad I misunderstood before! No issues, ignore me

No, you're right! This should be updated with at least a more descriptive message. (Also small bug) let me push one commit

[ashjbarnes]

Does the output print out that the conversion was done twice? Might be nice to have some kind of warning for this

No it doesn't print out twice. I've turned off the manual conversion for now, and the pint convert turns all of the zeros to -273. Does this happen for you too? Or is there something different about our workflows?

On the main branch, the land points remain 0. Presumably I just need to leave them as NaNs for a while longer, and fill with zeros after the pint convert

[manishvenu]

@ashjbarnes Looks like all the tests are passing in the CI with some minor changes to mom6_bathy if this PR needs to be merged. It's still very tentative, so still probably best to wait until we get it on pypi (or maybe being explicit this is a development version)!

This should mean the installation problems are fixed hopefully!

[ashjbarnes]

Thanks @manishvenu ! Next week I'll focus on this and try get this merged. Hopefully now that NCAR's mom6_bathy tools works for us, it will just be a matter of giving the main branch of your repo as a dependency (as you've done) and ironing out whatever's going on with the pytests

Could call and chat next week if that would be helpful / it's more complicated than I think!

[manishvenu]

Thanks @manishvenu ! Next week I'll focus on this and try get this merged. Hopefully now that NCAR's mom6_bathy tools works for us, it will just be a matter of giving the main branch of your repo as a dependency (as you've done) and ironing out whatever's going on with the pytests

Could call and chat next week if that would be helpful / it's more complicated than I think!

Hey Ashley,

I am working on updating the python version so that we can pass tests (visualCaseGen can't update python because of an issue so it needs to be updated). We can def call next week to wrap this up, but let me finish that first. I'll make sure to get the tests passing.

Thanks,
Manish

[manishvenu]

@ashjbarnes Once this most recent PR is merged, we should be good to go, mom6_bathy was renamed to forge & passes the tests

[ashjbarnes]

One holdup is I need to make sure that we can deploy this properly on gadi with the dependency being a github repo. Hopefully it's fine, but there's a bit more faff than usual because can't just test it on gadi, I also need to make sure that when the curated python environments (to which regional-mom6 belongs) are updated to new regional-mom6 version, it successfully picks up and installs the github repo. I've reached out to the ACCESS-NRI people for help and clarification :)

[ashjbarnes]

ok one issue I've found so far:

It looks like the supergrid as written by mom6_forge is missing the tile attribute. This (I think) is only needed for people using the FRE-NCtools. Using mom6_forge derived hgrid, I get:

error in get field_id of variable tile from file hgrid.nc NetCDF: Variable not found

when running the check_mask tool. Do you still use FRE NCtools at all any more @manishvenu ? If we decide as a community to no longer support these runs that's a possibility - we could point them to GFDL's regional mom6 ecosystem. Ideally, we'd find a way to keep things compatible with people who use FREtools / FMS though

These lines used to be included before writing the hgrid to disk.

edit: hmm this comment

suggests that mom6_forge is meant to be compatible with FRE tools? I'm then either misinterpreting something, or there's an issue with the lack of the tile attribute above since that does seem to cause our FRE tools to fail

[ashjbarnes]

Ok my thinking is that adding these silly tile attributes and dimensions to things for the minority of users who will continue using FREtools is silly. What I'll do instead is just add the tile attributes to files inside the run_fre_tools method. This means that you only get the tile stuff if you actually run the fre tools. I'll do a simple check in the FRE tools function to see if you already have tiles, and if not modify the hgrid & topo right then and there

[ashjbarnes]

Last commit ensures FMS / FRE tool backward compatibility. Instead of modifying mom6_forge to shoehorn in the tile dimension that FRE tools requires, now the run_fre_tool method patches the hgrid & bathymetry when run. So change will only affect FMS users.

I've also added comments in the demo files to explain that we're now less committed to maintain FMS runs generally

[ashjbarnes]

This works great - tested with OM2 forcing. The resultant input files are identical to the current main branch numerically, with some small changes to encoding. e.g., bathymetry now has coords named x/y instead of lat/lon. This is consistent with naming of the initial conditions anyway, and doesn't affect anything except how users plot them in notebooks.

I've now tested this workflow with FMS runs too, and with some tweaks to the run_fre_tools function, this is now backwards compatible.

Contrary to what I said this morning, I'm happy for this to be merged! Turns out that regional-mom6 has already been kicked off the curated analysis env since we're still requiring numpy <2 so no-one will even know if we can't load mom6_forge properly! I'll work on the bespoke regional-mom6 deployment env next week. #

[ashjbarnes]

but I'll let you merge this time...

[manishvenu]

Ok awesome! I'll merge this soon then!