Mom5 laplacians #27

arthurBarthe · 2021-02-22T20:34:05Z

This adds the MOM5 laplacians and the corresponding grid types. The PR is not ready for now but at least we thought we'd show where we are at.

One grid type for tracer cells and another for velocity cells, and one laplacian for each. Probably not ideal. We could have a dict of laplacians for each grid maybe, and the appropriate laplacian can be selected on calling the apply method depending on the passed field?
Unit tests of conservation fail unless using regular grid and no continental points. Elizabeth and I will try to figure out what's going on.

Merge branch 'master' of https://github.com/Zanna-ResearchTeam/gcm-filters

rabernat · 2021-03-04T17:16:58Z

Do we feel this is ready for me to review? We agree this is the way to go for MOM5?

rabernat

I just reviewed this and it looks 👍 to me! Tests are passing, code looks clean. So it seems we are ready to merge.

My only concern is about the notebook. Was it your intention to add the notebook to the documentation? If so, it needs to go in the docs folder and get linked from index.rst.

Furthermore, this notebook is quite long, and there are some sections that I'm not sure belong in the docs (e.g. "Our implementation of the MOM5 Laplacian (B-grid) for tracer fields".) To document the details of the Laplacian, you can put this information right in the documentation string (including latex equations) for the new Laplacian classes.

Once the notebooks is cleaned up and put in the right place, then I think we can merge this.

Any other comments from anyone else?

rabernat · 2021-03-16T18:32:27Z

gcm_filters/kernels.py

@@ -51,6 +58,95 @@ def __call__(self, field: ArrayType):
 ALL_KERNELS[GridType.CARTESIAN] = CartesianLaplacian


+@dataclass
+class MOM5LaplacianU(BaseLaplacian):
+    """Laplacian for MOM5 gird (velocity points)"""


Add description and equations here.

gcm_filters/kernels.py

arthurBarthe · 2021-03-22T13:18:48Z

Thanks for the review @rabernat I had missed that message sorry. I think we can happily leave the notebook aside as it is probably more suited to the paper. Does that make sense @NoraLoose? Also yes I'll put that in the post unit and submit and then it'll be ready for merging.

NoraLoose · 2021-09-01T17:41:38Z

@arthurBarthe, @ElizabethYankovsky, and @LaureZanna: it would be great to merge this PR before we submit the JOSS paper. Whenever you have the chance, would you mind making the changes discussed above?

In addition, it would be great to add your MOM5 Laplacian to the table of Laplacians in our documentation. This can be done by modifying docs/basic_filtering.rst.

Let me know how I can help. Thanks!

LaureZanna · 2021-09-04T11:12:46Z

Thanks @NoraLoose for the ping. Last month I checked with @arthurBarthe and @ElizabethYankovsky who told me that MOM5 was not going to be included in gcm-filter, glad to see that this has changed :-). @arthurBarthe has started a new faculty job but @ElizabethYankovsky and I can make the changes on Tue or Wed! We will surely ping you for help. Thanks!!

NoraLoose · 2021-09-04T16:21:39Z

Last month I checked with @arthurBarthe and @ElizabethYankovsky who told me that MOM5 was not going to be included in gcm-filter, glad to see that this has changed :-).

@LaureZanna, this must have been a misunderstanding. The last time we talked about this PR was in our meeting on May 5th (as far as I remember). And I thought we all agreed that we want to merge the MOM5 Laplacians, and that this PR just needs some minor modifications, as discussed above. Sorry if there was some miscommunication.

I would be more than happy to help you and @ElizabethYankovsky to finalize this PR. The code has advanced quite a bit in the last few months, so we will have to resolve some conflicts. On Tuesday I will be available until around 2pm Eastern, but then I will have to leave to the airport to catch a flight to Europe. I will be fully back online in the week after, starting September 13th (though in a time zone 6 hours ahead of you).

Resolving conflicts

review-notebook-app · 2021-09-10T14:43:14Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov-commenter · 2021-09-10T14:49:03Z

Codecov Report

Merging #27 (a264e37) into master (d352bec) will increase coverage by 0.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   98.25%   98.72%   +0.47%     
==========================================
  Files           9        9              
  Lines         861      944      +83     
==========================================
+ Hits          846      932      +86     
+ Misses         15       12       -3

Flag	Coverage Δ
unittests	`98.72% <100.00%> (+0.47%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/conftest.py	`100.00% <ø> (ø)`
tests/test_filter.py	`100.00% <ø> (ø)`
gcm_filters/kernels.py	`99.28% <100.00%> (+0.18%)`	⬆️
gcm_filters/filter.py	`98.98% <0.00%> (+1.67%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d352bec...a264e37. Read the comment docs.

NoraLoose · 2021-10-13T20:17:03Z

Thanks for the making the changes, @ElizabethYankovsky! Is this ready for another round of review?

NoraLoose

Thanks @ElizabethYankovsky! This looks good to me. I just left a few comments.

Another question is whether you meant to add the notebook MOM5_Laplacian_Filter.ipynb (part of this PR) to the documentation? I think @arthurBarthe mentioned that he wanted to remove it from this PR. If you'd like to keep the notebook, please let me know and I will review it.

NoraLoose · 2021-10-13T21:50:28Z

docs/basic_filtering.rst

@@ -38,6 +38,10 @@ The following table provides an overview of these different grid type options: w
 | ``IRREGULAR_WITH_LAND``        | locally orthogonal grid                                         | yes          | periodic           | Scalar Laplacian | :doc:`examples/example_filter_types`;    |
 |                                |                                                                 |              |                    |                  | :doc:`examples/example_tripole_grid`     |
 +--------------------------------+-----------------------------------------------------------------+--------------+--------------------+------------------+------------------------------------------+
+| ``MOM5U``                      | Velocity-point on Arakawa B-Grid                                | yes          | tripole            | Scalar Laplacian |                                          |


As of now, the Laplacians MOM5U and MOM5T do not use a tripole boundary condition (even though this could be easily done following the implementation for the TRIPOLAR_POP_WITH_LAND Laplacian). I think we should change tripole --> periodic.

NoraLoose · 2021-10-13T21:50:31Z

docs/basic_filtering.rst

@@ -38,6 +38,10 @@ The following table provides an overview of these different grid type options: w
 | ``IRREGULAR_WITH_LAND``        | locally orthogonal grid                                         | yes          | periodic           | Scalar Laplacian | :doc:`examples/example_filter_types`;    |
 |                                |                                                                 |              |                    |                  | :doc:`examples/example_tripole_grid`     |
 +--------------------------------+-----------------------------------------------------------------+--------------+--------------------+------------------+------------------------------------------+
+| ``MOM5U``                      | Velocity-point on Arakawa B-Grid                                | yes          | tripole            | Scalar Laplacian |                                          |
+--------------------------------+-----------------------------------------------------------------+--------------+--------------------+------------------+------------------------------------------+
+| ``MOM5T``                      | Tracer-point on Arakawa B-Grid                                  | yes          | tripole            | Scalar Laplacian |                                          |


Same here: tripole --> periodic

NoraLoose · 2021-10-13T21:51:23Z

gcm_filters/kernels.py

@@ -302,6 +304,116 @@ def __call__(self, field: ArrayType):
 ALL_KERNELS[GridType.IRREGULAR_WITH_LAND] = IrregularLaplacianWithLandMask


+@dataclass
+class MOM5LaplacianU(BaseScalarLaplacian):
+    """Laplacian for MOM5 (tracer points).


tracer points --> velocity points

NoraLoose · 2021-10-13T22:03:00Z

gcm_filters/kernels.py

+        self.y_wet_mask = self.wet_mask * np.roll(self.wet_mask, -1, axis=-2)
+
+    def __call__(self, field: ArrayType):
+        """Uses code by Elizabeth"""


I would remove this line.

ElizabethYankovsky · 2021-10-20T18:35:09Z

@NoraLoose thank you for the comments! I've updated the code with the changes you requested made. Please let me know if there are any other problems you notice. And yes, we can remove MOM5_Laplacian_Filter.ipynb from this PR -- where would be a good place to put this notebook? I can add it to the documentation and make sure it still works with the latest version of the package.

NoraLoose · 2021-10-21T16:35:04Z

Thanks @ElizabethYankovsky! Updated code and documentation look good to me!

I just had a quick look at the notebook MOM5_Laplacian_Filter.ipynb. It seems to be documenting the implementation of the Laplacian, which is an ingredient into the package but nothing that the user will interact with directly. I think for our documentation pages, users would find it maybe more helpful to see examples of how to use gcm-filters, rather than what is happening "under the hood"? Let me know what you think.
I'm happy to work with you on a MOM5 filtering notebook for the docs (maybe an example similar to what went into the JAMES paper). We could also defer such a notebook to later, and merge this PR as is (- MOM5_Laplacian_Filter.ipynb).

rabernat · 2021-10-21T16:40:30Z

It seems to be documenting the implementation of the Laplacian, which is an ingredient into the package but nothing that the user will interact with directly. I think for our documentation pages, users would find it maybe more helpful to see examples of how to use gcm-filters, rather than what is happening "under the hood"?

I agree 💯 with this. The documentation should not document the implementation. It should document how to use the package.

🙏 to @ElizabethYankovsky and @NoraLoose for the excellent work on this.

ElizabethYankovsky · 2021-10-28T18:10:52Z

Thanks @NoraLoose and @rabernat! I've removed the notebook, and agree that it will be more useful to add an example to the documentation pages of how gcm-filters can be used on a MOM5 grid. @NoraLoose please feel free to merge this PR as it is now.

For the JAMES paper I ended up re-rewriting the notebook to first interpolate all fields onto a C-grid (using the xgcm package) and then applying the vector laplacian formulated for a C-grid. However, in my original version of that notebook there was an example of B-grid usage. We can work on including this in the documentation (@NoraLoose I'll message you about this separately).

NoraLoose · 2021-10-28T18:42:08Z

Sounds great, @ElizabethYankovsky!

I think this PR is ready to be merged. We can wait a little bit longer to give @rabernat (and potentially others) a chance to also approve the changes.

Thanks @ElizabethYankovsky, @arthurBarthe, and @LaureZanna for this contribution!

LaureZanna · 2021-10-29T01:03:56Z

Thanks @NoraLoose and @rabernat!! this is amazing work. It was a lot of fun to play with the MOM5 kernels and getting involved in the coding with Arthur + Elizabeth. We are also using it for our ML work with Andrew Ross (and people in M2LInES).

arthurBarthe and others added 30 commits January 27, 2021 15:41

add mom5 folder

98033d6

Adding function to read the data

e1408b9

first version

6ad6943

Adding MOM5_Laplacian_filter notebook

9d2b597

Fixed comment on the dxt and dyt

167245d

Add own implementation of gaussian filter

c985446

Merge remote-tracking branch 'origin/master'

b107a58

Merge remote-tracking branch 'upstream/master'

885123d

Update for Elizabeth

14cacc8

Update for Elizabeth

5699882

Update for Elizabeth

4450c16

update example

4ed27f0

Adding the MOM5Laplacian

f02c848

Pulling gcm-filters

b79ae22

Merge branch 'master' of https://github.com/Zanna-ResearchTeam/gcm-filters

Updates for iterative laplacian

de21e51

First version

d6b5c68

Correcting Laplacian operator in notebook, add vorticity

640c92e

Correcting kernels.py and adding area factor

59bbc5b

First working version of Laplacian-based filtering

3aef89f

Use array computations for Laplacian

6962134

Minor fix

effdea6

Add cartesian filter, plots to same format

546e9eb

Adding LAP_T and vorticity function, correcting LAP_U

580be9c

Updating local branch

640e2f0

Merge branch 'master' of https://github.com/Zanna-ResearchTeam/gcm-filters

Update of MOM5 Laplacian notebook

fa5f642

Minor fix

14006cb

Merge remote-tracking branch 'origin/master'

69bda64

Allow specify data location in parameters

c85a913

add MOM5 laplacian

0e7f444

fix merge conflicts

9992845

rabernat requested changes Mar 16, 2021

View reviewed changes

NoraLoose reviewed Mar 16, 2021

View reviewed changes

gcm_filters/kernels.py Show resolved Hide resolved

NoraLoose added 2 commits September 10, 2021 11:23

Merge branch 'master' into MOM5_laplacians

1bd2b72

Add MOM5 Laplacians to test suite

8ca345a

NoraLoose mentioned this pull request Sep 10, 2021

Resolving conflicts Zanna-ResearchTeam/gcm-filters#3

Merged

Merge pull request #3 from NoraLoose/MOM5_laplacians_conflicts_resolved

7e0a285

Resolving conflicts

Elizabeth A Yankovsky added 2 commits September 17, 2021 17:54

Adding attribute descriptions

966f207

Adding documentation to GridType table

4b0c2ea

NoraLoose requested changes Oct 13, 2021

View reviewed changes

NoraLoose reviewed Oct 13, 2021

View reviewed changes

Resolving pre-commit-hook changes

38237f0

Fixing pre-commit

414c557

Removing notebook

a264e37

NoraLoose approved these changes Oct 28, 2021

View reviewed changes

rabernat approved these changes Oct 29, 2021

View reviewed changes

rabernat merged commit d5d1301 into ocean-eddy-cpt:master Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mom5 laplacians #27

Mom5 laplacians #27

arthurBarthe commented Feb 22, 2021

rabernat commented Mar 4, 2021

rabernat left a comment

rabernat Mar 16, 2021

arthurBarthe commented Mar 22, 2021

NoraLoose commented Sep 1, 2021

LaureZanna commented Sep 4, 2021

NoraLoose commented Sep 4, 2021 •

edited

Loading

review-notebook-app bot commented Sep 10, 2021

codecov-commenter commented Sep 10, 2021 •

edited

Loading

NoraLoose commented Oct 13, 2021

NoraLoose left a comment

NoraLoose Oct 13, 2021

NoraLoose Oct 13, 2021

NoraLoose Oct 13, 2021

NoraLoose Oct 13, 2021

ElizabethYankovsky commented Oct 20, 2021

NoraLoose commented Oct 21, 2021

rabernat commented Oct 21, 2021 •

edited

Loading

ElizabethYankovsky commented Oct 28, 2021

NoraLoose commented Oct 28, 2021

LaureZanna commented Oct 29, 2021

Mom5 laplacians #27

Mom5 laplacians #27

Conversation

arthurBarthe commented Feb 22, 2021

rabernat commented Mar 4, 2021

rabernat left a comment

Choose a reason for hiding this comment

rabernat Mar 16, 2021

Choose a reason for hiding this comment

arthurBarthe commented Mar 22, 2021

NoraLoose commented Sep 1, 2021

LaureZanna commented Sep 4, 2021

NoraLoose commented Sep 4, 2021 • edited Loading

review-notebook-app bot commented Sep 10, 2021

codecov-commenter commented Sep 10, 2021 • edited Loading

Codecov Report

NoraLoose commented Oct 13, 2021

NoraLoose left a comment

Choose a reason for hiding this comment

NoraLoose Oct 13, 2021

Choose a reason for hiding this comment

NoraLoose Oct 13, 2021

Choose a reason for hiding this comment

NoraLoose Oct 13, 2021

Choose a reason for hiding this comment

NoraLoose Oct 13, 2021

Choose a reason for hiding this comment

ElizabethYankovsky commented Oct 20, 2021

NoraLoose commented Oct 21, 2021

rabernat commented Oct 21, 2021 • edited Loading

ElizabethYankovsky commented Oct 28, 2021

NoraLoose commented Oct 28, 2021

LaureZanna commented Oct 29, 2021

NoraLoose commented Sep 4, 2021 •

edited

Loading

codecov-commenter commented Sep 10, 2021 •

edited

Loading

rabernat commented Oct 21, 2021 •

edited

Loading