-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
T-Laplacian for POP tripolar grid for varying dx, dy #29
Conversation
* 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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Is this ready for review? |
Yes, ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
gcm_filters/kernels.py
Outdated
"CARTESIAN", | ||
"CARTESIAN_WITH_LAND", | ||
"IRREGULAR_CARTESIAN_WITH_LAND", | ||
"POP_SIMPLE_TRIPOLAR_T_GRID", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if self.wet_mask[..., 0, :].any(): | ||
raise AssertionError("Wet mask requires zeros in southernmost row") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some profiling and decided that this is not actually a big performance hit. So we can leave things as are for now.
|
||
folded = field[..., [-1], :] # grab northernmost row | ||
folded = folded[..., ::-1] # mirror it | ||
field_extended = np.concatenate((field, folded), axis=-2) # append it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 perfect style 🤩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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? |
Hi Nora! Any chance we can think of a test that would have caught the sign error? If not, no big deal. |
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 |
Yes, that would be great! Thanks so much. |
I added two new tests in Edit: This is ready for review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
[ | ||
"REGULAR", | ||
"REGULAR_WITH_LAND", | ||
"IRREGULAR_WITH_LAND", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the grid types changing name, we will need to update the tutorial notebooks to be consistent.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment here explaining the sign convention of the shift
argument would be helpful to future developers.
tests/test_kernels.py
Outdated
################## Tripolar grid tests ############################################## | ||
tripolar_grids = [ | ||
member | ||
for name, member in GridType.__members__.items() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to use GridType.__members__.items()
here? Does for name in GridType
not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for name in GridType
raises the error:
TypeError: argument of type 'GridType' is not iterable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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']
tests/test_kernels.py
Outdated
# 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
Thanks for the review @rabernat!
|
I think I have made all changes you suggested. Please let me know if you can think of anything else. Thanks! |
Fantastic! Thanks again for your excellent work on this. I'm excited to keep pushing it forward! |
This adds the
POP_TRIPOLAR_T_GRID
and the associated LaplacianPOPTripolarLaplacianTpoint
. This addition is for creating general filters on the POP tripolar grid and needs the user to input stuff likedxe
,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 exceptwet_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.