T-Laplacian for POP tripolar grid for varying dx, dy #29

NoraLoose · 2021-02-24T22:02:23Z

This adds the POP_TRIPOLAR_T_GRID and the associated Laplacian POPTripolarLaplacianTpoint. This addition is for creating general filters on the POP tripolar grid and needs the user to input stuff like dxe, dye, dxn, dyn - grid length spacing of the eastern and northern T-cell edges. (In contrast, the Laplacian in PR #26 is only for fixed factor filtering and doesn't need any grid information except wet_mask.)

I also included a notebook that compares the action of the 5 different Laplacian kernels that we have so far (on POP model output), and compares the timing of these on CPU.

This PR sits on top of #26, so #26 should be merged first.

* get rid off "position" argument, and instead have two separate kernels POPTripolarSimpleLaplacianTpoint and POPTripolarSimpleLaplacianUpoint * comment out the check that U-field is folded properly (until this discussion in PR#26 is resoloved) # Please enter the commit message for your changes. Lines starting

* comment out conservation tests for tripolar U-points until we have figured out how/if we can impose this property (PR ocean-eddy-cpt#26)

codecov-io · 2021-02-24T22:05:32Z

Codecov Report

Merging #29 (79aef47) into master (ed1a007) will increase coverage by 1.58%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   96.22%   97.81%   +1.58%     
==========================================
  Files           7        7              
  Lines         265      457     +192     
==========================================
+ Hits          255      447     +192     
  Misses         10       10

Flag	Coverage Δ
unittests	`97.81% <100.00%> (+1.58%)`	⬆️

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

Impacted Files	Coverage Δ
gcm_filters/filter.py	`94.95% <ø> (+0.51%)`	⬆️
gcm_filters/kernels.py	`100.00% <100.00%> (ø)`
tests/test_filter.py	`100.00% <100.00%> (ø)`
tests/test_kernels.py	`100.00% <100.00%> (ø)`

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 ed1a007...79aef47. Read the comment docs.

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

Is this ready for review?

NoraLoose · 2021-03-04T17:25:05Z

Yes, ready for review.

rabernat

This is really excellent work Nora! I tried hard to find areas to improve your code but could not! 😊 I especially like your tests for the tripolar boundary condition.

My only question is about our terminology to describe these different grids.

rabernat · 2021-03-04T17:45:56Z

gcm_filters/kernels.py

+        "CARTESIAN",
+        "CARTESIAN_WITH_LAND",
+        "IRREGULAR_CARTESIAN_WITH_LAND",
+        "POP_SIMPLE_TRIPOLAR_T_GRID",


This kernel strikes me as very similar to the CARTESIAN_WITH_LAND, the only difference being the boundary condition. Do we want to think about harmonizing the names? What about TRIPOLAR_CARTESIAN_WITH_LAND?

Or maybe we want to rename things like REGULAR, REGULAR_WITH_LAND, TRIPOLAR_REGULAR_WITH_LAND, etc?

Thanks, Ryan! Good point about harmonizing the names. I like your suggestion, and vote for: REGULAR, REGULAR_WITH_LAND, IRREGULAR_WITH_LAND, TRIPOLAR_REGULAR_WITH_LAND, TRIPOLAR_IRREGULAR_WITH_LAND. Or we could keep the latter as POP_TRIPOLAR_T_GRID, and change it later to something more generic (e.g., TRIPOLAR_IRREGULAR_WITH_LAND), as you suggested here: ocean-eddy-cpt/gcm-filters-paper#18 (comment)

I think we should some distinguish between three types of grids:

Regular + Rectangular: dx=dy=const, dimensions are globally orthogonal (what VTK calls "image data")

Irregular + Rectangular, dimensions are globally orthogonal, but dx and dy can vary independently (what VTK calls "rectilinear"

Curvilinear, dimensions are locally orthogonal, but the grid curves (what vtk calls "structure")

Unstructured mesh (e.g. triangles, hexagons, etc.) - probably out of scope for now

In addition to this, we seem to have two independent option spaces

Boundary conditions (e.g. singly or doubly periodic, tripolar, cubed-sphere, etc.)

Land mask or not

(VTK reference: http://www.bu.edu/tech/support/research/training-consulting/online-tutorials/vtk/)

We don't need to resolve all of this now, but maybe we can use these concepts to inform the naming convention.

Down the road (future PR), we could maybe even use multiple inheritance to define kernels, like

class POPCurvilinearGrid(CurvilinearGrid, GridWithLandMask, TripolarBoundaryCondition): pass

Agreed, these are useful guidelines.

The thing is that all Laplacians that we have implemented so far can be used on curvilinear grids, even the ones where dx=dy=1, e.g., CARTESIAN_WITH_LAND or POP_SIMPLE_TRIPOLAR_T_GRID. The latter are the Laplacians for fixed factor filtering, but they work for the POP curvilinear grid, for instance.

I like your idea about multiple inheritance to define kernels - this is more or less what I have imagined too. But I appreciate that this is maybe more of a long-term goal, and will come with its own challenges.

Since all Laplacians are good for curvilinear grids, I decided to go with

"regular": Laplacian needs no grid info (for fixed factor filtering)

"irregular": Laplacian needs grid info (for more general filtering, e.g., with fixed scale)

I'm happy to change this if there are better suggestions, either now or later.

…ead)

NoraLoose · 2021-03-15T21:44:11Z

gcm_filters/kernels.py

+        if self.wet_mask[..., 0, :].any():
+            raise AssertionError("Wet mask requires zeros in southernmost row")


@rabernat, related to your comment on checks here, I'm assuming you would suggest to remove this check (and associated test) as well. I added this check because this is a global Laplacian (on a tripole grid), but the southern boundary must be land in order to disable the periodic boundary condition in y-direction. Otherwise, the filtered values in the very south will be contaminated by stuff in the Arctic.

Again, an alternative is to document the requirement of wet mask zeros in the southernmost row carefully, and I am happy to remove this check.

Good point.

I will think about this and get back to you. Perhaps fixing the problem at the root (only call __post_init__ once) would be the best path forward.

I did some profiling and decided that this is not actually a big performance hit. So we can leave things as are for now.

rabernat · 2021-03-16T18:36:53Z

gcm_filters/kernels.py

+
+    folded = field[..., [-1], :]  # grab northernmost row
+    folded = folded[..., ::-1]  # mirror it
+    field_extended = np.concatenate((field, folded), axis=-2)  # append it


👌 perfect style 🤩

rabernat

This is great work. I think we can merge it.

- correct np.roll(field, +1, axis=ax) --> np.roll(field, -1, axis=ax) and vice versa - this bug was present in two Laplacians: * IRREGULAR_WITH_LAND * TRIPOLAR_POP_WITH_LAND

NoraLoose · 2021-03-22T20:16:07Z

Good that we hadn't merged this. I discovered a sign error in my rolling operations (which I made so consistently throughout my code that the tests did not uncover it). I fixed it, and we should now be ready to merge.

rabernat · 2021-03-22T21:43:16Z

Nice!

Can you think what sort of test would have caught your sign error? Is there a new type of test we should consider adding to the kernel test suite?

rabernat · 2021-03-30T12:32:14Z

Is there a new type of test we should consider adding to the kernel test suite?

Hi Nora! Any chance we can think of a test that would have caught the sign error? If not, no big deal.

NoraLoose · 2021-03-30T14:56:20Z

Yes, I think I have an idea. I have time to work on this today.

If I come up with a successful test, would you like me to submit it as part of this PR? (I would design the test for all irregular Laplacians, i.e., all Laplacians that need grid info dx, dy.)

rabernat · 2021-03-30T14:56:54Z

If I come up with a successful test, would you like me to submit it as part of this PR?

Yes, that would be great! Thanks so much.

NoraLoose · 2021-03-30T18:06:09Z

I added two new tests in test_kernels.py that would have caught the sign error: test_flux_in_y_direction, test_flux_in_x_direction. I verified that the old version of the code (before commit ba228f7) would have failed the test.

Edit: This is ready for review now.

rabernat

Thanks again @NoraLoose for this amazing PR! And sorry for being slow to review.

My only real concern that, if we are changing the names of things, our documentation tutorials need to be updated for consistency. In https://gcm-filters.readthedocs.io/en/latest/tutorial.html#create-a-filter for example we still use CARTESIAN_WITH_LAND, etc.

Also a few nit-picking code style issues.

With this we will finally be ready to merge. Thanks for your patience and your excellent work.

rabernat · 2021-04-01T14:21:02Z

gcm_filters/kernels.py

+    [
+        "REGULAR",
+        "REGULAR_WITH_LAND",
+        "IRREGULAR_WITH_LAND",


For the grid types changing name, we will need to update the tutorial notebooks to be consistent.

rabernat · 2021-04-01T15:59:32Z

gcm_filters/kernels.py

-        self.w_wet_mask = self.wet_mask * np.roll(self.wet_mask, -1, axis=-1)
-        self.s_wet_mask = self.wet_mask * np.roll(self.wet_mask, -1, axis=-2)
+        self.w_wet_mask = self.wet_mask * np.roll(self.wet_mask, 1, axis=-1)
+        self.s_wet_mask = self.wet_mask * np.roll(self.wet_mask, 1, axis=-2)


Perhaps a comment here explaining the sign convention of the shift argument would be helpful to future developers.

rabernat · 2021-04-01T16:02:04Z

tests/test_kernels.py

+################## Tripolar grid tests ##############################################
+tripolar_grids = [
+    member
+    for name, member in GridType.__members__.items()


Why do you need to use GridType.__members__.items() here? Does for name in GridType not work?

for name in GridType raises the error:

TypeError: argument of type 'GridType' is not iterable

That is puzzling to me. GridType is an enum. The following works.

import enum GridType = enum.Enum("GRID_TYPE", ["A", "B"]) assert [gt.name for gt in GridType] == ["A", 'B']

rabernat · 2021-04-01T16:02:38Z

tests/test_kernels.py

+# would be: all grids that have len(required_grid_vars)>1 (more than just a wet_mask)
+irregular_grids = [
+    member
+    for name, member in GridType.__members__.items()


NoraLoose · 2021-04-01T16:23:33Z

Thanks for the review @rabernat!

I will add some comments on the sign convention of the shift argument, as you suggest.
Re the Laplacian name changes: I'm happy to update the tutorials. Do we think that the new naming convention is superior to the old one? It does shorten the names by a little bit...

NoraLoose · 2021-04-01T21:23:26Z

I think I have made all changes you suggested. Please let me know if you can think of anything else. Thanks!

rabernat · 2021-04-01T21:44:09Z

Fantastic! Thanks again for your excellent work on this. I'm excited to keep pushing it forward!

NoraLoose added 21 commits February 19, 2021 19:07

Add simple Laplacian for POP tripolar grid + tests

fa1981f

Add simple Laplacian for POP tripolar grid + tests

478cf91

save memory in POPTripolarSimpleLaplacian computation

3656b36

add notebook that documents the POP tripolar grid Laplacian

d200dd6

add POP tripolar Laplacian for U-fields

a68ac41

add tripolar grid tests

8639a29

attempt to hack filter.py to allow grid_vars to contain string

8c0210d

include POP_SIMPLE_TRIPOLAR_GRID in filter tests

f74d46d

clean up comments in filter.py

7ade259

Update fixtures to include newly added Laplacians

ddd19f9

Update fixtures and kernel.py tests

bd23cbb

* comment out conservation tests for tripolar U-points until we have figured out how/if we can impose this property (PR ocean-eddy-cpt#26)

all rebased and run through pre-commit

343640f

add @DataClass line that got lost while rebasing

9c608f8

some cosmetic changes

3e3edd8

remove jupyter notebook with performance tests

6e23bd4

remove POP_SIMPLE_TRIPOLAR_U_GRID and associated Laplacian and tests

a47d96b

Add most general POP tripolar Laplacian on T-points

a8b855e

Update tests to include new POP tripolar Laplacian

ece1246

delete unneeded code in test modules

b863209

add notebook with Laplacian and filter timing tests

ab8f192

NoraLoose mentioned this pull request Feb 24, 2021

section on computational aspects ocean-eddy-cpt/gcm-filters-paper#4

Closed

NoraLoose added 2 commits February 26, 2021 08:58

delete jupyter notebook with timing tests

77c9f53

re-add identical jupyter notebook deleted in previous commit

926ce07

rabernat mentioned this pull request Mar 4, 2021

Eddy Momentum Example on MOM5 data ocean-eddy-cpt/gcm-filters-paper#18

Closed

rabernat mentioned this pull request Mar 4, 2021

Laplacian on POP tripolar grid #26

Closed

rabernat reviewed Mar 4, 2021

View reviewed changes

NoraLoose added 2 commits March 4, 2021 14:16

harmonize grid names

b2e1b5a

remove notebook with timing tests (add to gcm-filters-paper repo inst…

0591b7a

…ead)

NoraLoose mentioned this pull request Mar 11, 2021

Read and comment before 3/22. ocean-eddy-cpt/gcm-filters-paper#34

Closed

NoraLoose commented Mar 15, 2021

View reviewed changes

rabernat reviewed Mar 16, 2021

View reviewed changes

rabernat approved these changes Mar 16, 2021

View reviewed changes

Fix sign error in rolling operation

ba228f7

- correct np.roll(field, +1, axis=ax) --> np.roll(field, -1, axis=ax) and vice versa - this bug was present in two Laplacians: * IRREGULAR_WITH_LAND * TRIPOLAR_POP_WITH_LAND

NoraLoose added 2 commits March 30, 2021 11:48

Add tests that would catch sign error in Laplacian rolling operation

f69cbff

Clean up description of new test

47aba9a

Rename new tests and provide more fitting description

82d4720

rabernat requested changes Apr 1, 2021

View reviewed changes

NoraLoose added 3 commits April 1, 2021 12:49

Simplify code for definition of irregular/tripolar grid types

ae0b0d7

Add comments to explain sign convention of shift argument

4b89aa5

Update Laplacian names in documentation tutorials

79aef47

rabernat merged commit f3f644a into ocean-eddy-cpt:master Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T-Laplacian for POP tripolar grid for varying dx, dy #29

T-Laplacian for POP tripolar grid for varying dx, dy #29

NoraLoose commented Feb 24, 2021

codecov-io commented Feb 24, 2021 •

edited

Loading

rabernat commented Mar 4, 2021

NoraLoose commented Mar 4, 2021

rabernat left a comment

rabernat Mar 4, 2021

NoraLoose Mar 4, 2021

rabernat Mar 4, 2021

NoraLoose Mar 4, 2021

NoraLoose Mar 4, 2021

NoraLoose Mar 15, 2021

rabernat Mar 15, 2021

rabernat Mar 16, 2021

rabernat Mar 16, 2021

rabernat left a comment

NoraLoose commented Mar 22, 2021

rabernat commented Mar 22, 2021

rabernat commented Mar 30, 2021

NoraLoose commented Mar 30, 2021

rabernat commented Mar 30, 2021

NoraLoose commented Mar 30, 2021 •

edited

Loading

rabernat left a comment

rabernat Apr 1, 2021

rabernat Apr 1, 2021

rabernat Apr 1, 2021

NoraLoose Apr 1, 2021

rabernat Apr 1, 2021 •

edited

Loading

rabernat Apr 1, 2021

NoraLoose commented Apr 1, 2021

NoraLoose commented Apr 1, 2021

rabernat commented Apr 1, 2021

		if self.wet_mask[..., 0, :].any():
		raise AssertionError("Wet mask requires zeros in southernmost row")

T-Laplacian for POP tripolar grid for varying dx, dy #29

T-Laplacian for POP tripolar grid for varying dx, dy #29

Conversation

NoraLoose commented Feb 24, 2021

codecov-io commented Feb 24, 2021 • edited Loading

Codecov Report

rabernat commented Mar 4, 2021

NoraLoose commented Mar 4, 2021

rabernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabernat left a comment

Choose a reason for hiding this comment

NoraLoose commented Mar 22, 2021

rabernat commented Mar 22, 2021

rabernat commented Mar 30, 2021

NoraLoose commented Mar 30, 2021

rabernat commented Mar 30, 2021

NoraLoose commented Mar 30, 2021 • edited Loading

rabernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabernat Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoraLoose commented Apr 1, 2021

NoraLoose commented Apr 1, 2021

rabernat commented Apr 1, 2021

codecov-io commented Feb 24, 2021 •

edited

Loading

NoraLoose commented Mar 30, 2021 •

edited

Loading

rabernat Apr 1, 2021 •

edited

Loading