search

MOM6-community / sectionate

compute section in ocean models
GNU General Public License v3.0
8 stars 7 forks source link

Generalized `MOM6_normal_transport` to `MOM6_convergent_transport` and many quality-of-life improvements #11

Closed hdrake closed 1 year ago

hdrake commented 2 years ago

This is pretty much ready to go. It reproduces the results of the current example notebooks for the OSNAP arrays (at least visually based on the plots, and I think exactly, but I did not actually check).

The most important change is that MOM6_normal_transport has now been generalized and handles all section orientations in a self-consistent way, whereas the previous implementation requires sections to be monotonically east-to-west and gave northward transport across those sections. Now sections can be arbitrary and can be closed boundaries of regions in addition to open sections (like the OSNAP exampels).

The new version handles both symmetric and non-symmetric grids right out of the box (just flip the boolean keyword argument symmetric) rather than relying on offsets.

I should at some point include an example for a close region.

hdrake commented 2 years ago

Would be great if @raphaeldussin and @MDTocean could review!

raphaeldussin commented 2 years ago

@hdrake thanks for this great contribution! My main concern with this PR is that it deletes large portions of functions that we may want to obsolete in a more progressive way and even maybe keep as consistency checks. Would you be open to rename the old functions with say _old or include an old_algo logical in there?

hdrake commented 2 years ago

Thanks @raphaeldussin. What if I did a few intermediate commits that keep the old methods and use unit tests to show they are equivalent? As I said above, I don't really see the purpose of keeping any of the old ones in long term (they are either redundant or did not serve a clear purpose), especially because there is always a record of them in git and the code could be reverted/reintroduced if necessary.

Although I deleted a lot of code, all of the functionality that was demonstrated in the examples notebooks is still there! I think the only breaking change is that MOM6_normal_transport is now renamed to MOM6_convergent_transport but even the default arguments are not a breaking change. Is there a particular removed function / feature you are concerned about?

hdrake commented 1 year ago

Included in https://github.com/raphaeldussin/sectionate/pull/16