Handle simple fixed factor filtering internally #75

NoraLoose · 2021-06-11T22:29:37Z

Issue

This PR fixes issue #71.

For applying a simple fixed factor filter (as for example described in the "Anisoptropic Filtering" section of the Filter Theory), the user was required to manually go through the following steps:

Before filtering, multiply the field by the local cell area.
Apply gcm-filters filter with dx_min=1 and filter_scale= desired fixed factor, pretending the grid was uniform.
After filtering, divide filtered field by local cell area.

The first step is essentially a coordinate transformation where the original (locally orthogonal) grid is transformed to a uniform Cartesian grid with dx=dy=1. The third step is the reverse coordinate transformation.

Changes

These steps are now handled internally by the code.

I introduced a new Laplacian base class BaseScalarLaplacianWithArea for the Laplacians that are for simple fixed factor filtering:

TRANFORMED_TO_REGULAR
TRANSFORMED_TO_REGULAR_WITH_LAND
TRIPOLAR_TRANSFORMED_TO_REGULAR_WITH_LAND.

Steps 1 and 3 from above are handled as part of the filter class for all Laplacians that are a subclass of BaseScalarLaplacianWithArea.

Tutorial changes

I updated all tutorials to reflect the new way of doing simple fixed factor filtering.

Old:

filtered = filter.apply(field * area, dims=['yh', 'xh']) / area

New:

filtered = filter.apply(field, dims=['yh', 'xh'])

While updating the tutorials, I also fixed some typos and clarified some statements. Thanks @sdbachman for reading through the documentation and for providing these comments.

- This is a more realistic and stronger test

* Pass REGULAR and REGULAR_WITH_LAND Laplacian the additional grid variable "area" * Fix typos and implement Scott's comments

…into fixed-factor-filtering

…ters into fixed-factor-filtering

review-notebook-app · 2021-06-11T22:29:41Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov-commenter · 2021-06-11T22:33:26Z

Codecov Report

Merging #75 (e2ab6c9) into master (65294c7) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   98.47%   98.66%   +0.19%     
==========================================
  Files           7        7              
  Lines         719      824     +105     
==========================================
+ Hits          708      813     +105     
  Misses         11       11

Flag	Coverage Δ
unittests	`98.66% <100.00%> (+0.19%)`	⬆️

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

Impacted Files	Coverage Δ
gcm_filters/filter.py	`97.26% <100.00%> (+0.10%)`	⬆️
gcm_filters/kernels.py	`99.02% <100.00%> (+0.14%)`	⬆️
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 65294c7...e2ab6c9. Read the comment docs.

…ters into fixed-factor-filtering

iangrooms

These changes look good to me. @rabernat might want to take a look for Python style issues.

* before: REGULAR and REGULAR_WITH_LAND required an area grid variable that is needed for fixed factor filtering; this is reverted * instead: introduce two separate grid types TRANSFORM_TO_REGULAR and TRANSFORM_TO_REGULAR_WITH_LAND that are specifically for fixed factor filtering * expand docstrings of Laplacians * updated tests to mirror name changes

rabernat

First, let me apologize deeply and sincerely for taking so long to review this. The past month has been extremely challenging for me work-wise due to family and travel-related reasons.

I really like the spirit of this PR. I think it makes things much simpler to implement the weighting internally. However, I have one large-ish suggestions for the implementation.

I don't think it makes sense to check the type of the kernel in filters.py and then do things differently based on what we find. That is a coding pattern that indicates poor separation of concerns between modules. Instead, I think kernels.py should take care of the weighting.

I think the best way to accomplish this would be to expand the Kernel interface to include prepare and finalize methods in the BaseLaplacian classes.

The default would be to do nothing, e.g.:

    def prepare(self, field):
        return field

But implementations could override this, e.g.

@dataclass
class AreaWeightedLaplacian(BaseLaplacian)
    area: ArrayLike

    def prepare(self, field):
        return field * self.area

    def __call__(self, field):
        # do stuff

    def finalize(self, field):
        return field / self.area

This could even be a mixin, so we don't have to redefine the core laplacian __call__ functions, e.g.

@dataclass
class AreaWeightedMixin(BaseLaplacian)
    area: ArrayLike

    def prepare(self, field):
        return field * self.area

    def finalize(self, field):
        return field / self.area

@dataclass
class RegularLaplacianWithArea(AreaWeightedMixin, RegularLaplacian):
    pass

Then filter.py would just call field = kernel.prepare(field), rather than implementing the weighting directly.

Does this suggestion make sense? Happy to chat more, and sorry again for my slowness.

I really appreciate your diligent and careful work on this project.

rabernat · 2021-07-27T15:14:10Z

gcm_filters/filter.py

+                    "--> dx_min is set to 1",
+                    stacklevel=2,
+                )
+                self.dx_min = 1


What scenario are you imagining here with this check / warming? Rather than overwriting dx_min, why don't we just raise a ValueError here and make the user fix it explicitly?

rabernat · 2021-07-27T15:14:59Z

gcm_filters/filter.py

@@ -309,8 +328,8 @@ def __post_init__(self):
        ]
        if filter_factor >= max_filter_factor:
            warnings.warn(
-                "Warning: Filter scale much larger than grid scale -> numerical instability possible",
-                UserWarning,
+                "Filter scale much larger than grid scale -> numerical instability possible",


Can we link to a documentation page with more context?

rabernat · 2021-07-27T15:16:13Z

gcm_filters/filter.py

            raise ValueError(
                f"Provided Laplacian {self.Laplacian} is a vector Laplacian. "
                f"The ``.apply`` method is only suitable for scalar Laplacians."
            )
+        if issubclass(self.Laplacian, BaseScalarLaplacianWithArea):
+            # simple fixed factor filtering multiplies field by area before filtering
+            field = field * self.grid_ds["area"]


Rather than putting this login in the filter module, why not put it into the kernel itself?

rabernat · 2021-07-27T15:17:35Z

gcm_filters/kernels.py

+    """
+
+    area: ArrayType
+


There should be a way to make the class itself do the area weighting / deweighting. To me that would be a lot cleaner than doing it manually in the filter module (better separation of concerns).

* Introduce mixin class AreaWeightedMixin that handles area weighting and deweigting in kernel.py * Take advantage of multiple class inheritance in kernel.py for - RegularLaplacianWithArea - RegularLaplacianWithLandMaskAndArea * filter_func now calls prepare and finalize methods of the Laplacian classes * Update all tests

* ... if dx_min is not equal to 1 for simple fixed factor filtering * update test accordingly

NoraLoose · 2021-07-27T18:39:20Z

Instead, I think kernels.py should take care of the weighting.

I think the best way to accomplish this would be to expand the Kernel interface to include prepare and finalize methods in the BaseLaplacian classes.

Thanks for this suggestion, Ryan! I really like it; it makes things way cleaner.

I updated the PR, and incorporated all your comments and suggestions.

rabernat

Thanks for the super quick revisions! 🚀

One more round of minor comments, focused on the naming of stuff and documentation. Then LGTM!

gcm_filters/filter.py

rabernat · 2021-07-28T12:54:45Z

gcm_filters/kernels.py

+    1) Field on locally orthogonal grid is transformed to field on regularly spaced Cartesian
+    grid with dx = dy = 1, through multiplication by cell area of original grid.
+    2) Laplacian acts on regular Cartesian grid.
+    3) Diffused field on regular Cartesian grid is transformed back to field on original grid,
+    through division by cell area of original grid.


If you format this as a proper RST list, it will render correctly in the docs. As is, the list is "inline" and doesn't quite look right: https://gcm-filters--75.org.readthedocs.build/en/75/api.html#gcm_filters.kernels.RegularLaplacianWithArea

Thanks - the lists render correctly now.

I also tried to get rid off the hyphen that appears in the API at the beginning of the rendered docstrings for some of the Laplacians (but not for others). I couldn't resolve this issue. Any ideas?

I do not, but this is relatively minor, so not a big deal to me.

gcm_filters/kernels.py

rabernat · 2021-07-28T12:56:27Z

gcm_filters/kernels.py

+
+    Attributes
+    ----------
+    area: cell area of original grid


Suggested change

area: cell area of original grid

When you use the AreaWeightedMixin, you don't need area as part of this class.

rabernat · 2021-07-28T12:56:46Z

gcm_filters/kernels.py


    Attributes
    ----------
+    area: cell area of original grid


Suggested change

area: cell area of original grid

When you use the AreaWeightedMixin, you don't need area as part of this class.

True, except that the function required_grid_vars will not pick up area if I don't redefine it here.

This is only an issue for the classes RegularLaplacianWithLandMaskAndArea and TripolarRegularLaplacianTpoint where additional attributes have to be defined (in addition to what is inherited from the superclasses). In contrast, it is not an issue for the RegularLaplacianWithArea which does not need any additional attributes.

Ok that makes sense. I don't really understand why return list(self.__annotations__) in BaseScalarLaplacian does not pick up on the mixin attributes; it must have to do with the nitty-gritty details of class inheritance. Perhaps fixable somehow in the required_grid_args but not worth more effort here.

rabernat · 2021-07-28T13:00:18Z

gcm_filters/kernels.py

+    pass
+
+
+ALL_KERNELS[GridType.TRANSFORMED_TO_REGULAR] = RegularLaplacianWithArea


I find this naming confusing. "Transformed" is ambiguous, and it doesn't indicate anything about the area weighting. What aboutREGULAR_AREA_WEIGHTED?

This is not a dealbreaker for me...but I'm just trying to think about what is most obvious for users.

I actually went back and forth with the naming convention here; something similar to what you suggest here has been in the mix too! So I'm glad to hear that you find AREA_WEIGHTED most intuitive.

rabernat · 2021-07-28T13:00:51Z

gcm_filters/kernels.py

+
+
+ALL_KERNELS[
+    GridType.TRANSFORMED_TO_REGULAR_WITH_LAND


Same. Do we really want to call this "TRANSFORMED"? Or would "AREA_WEIGHTED" be more clear?

gcm_filters/kernels.py

* change Laplacian naming convention from TRANSFORMED_TO to AREA_WEIGHTED * update tests according to new naming convention * reformat docstrings in kernel.py module so lists show up properly in API * link docstrings to kernel methods

NoraLoose · 2021-07-28T16:50:15Z

Since I changed the names of 3 Laplacians (from TRANSFORMED_TO to AREA_WEIGHTED) I should also update all the example notebooks for the documentation. I will do that as soon as casper is up and running again.

I could also do the notebook update as part of PR #78, which is a big docs update.

rabernat · 2021-07-29T06:20:43Z

I could also do the notebook update as part of PR #78,

Let's go with that! I think we should merge this now.

NoraLoose added 22 commits June 7, 2021 18:07

Add area as grid variable to REGULAR

e1d5a25

Introduce new base class for simple fixed filter Laplacians

17189fc

Add area to fixture parameterizations for filter specs

c3a9291

Adapt filter error message to new base class

3860607

Implement simple fixed factor filtering in filter class

a1bf0f8

Change conservation tests to area-weighted integral

4e54e44

Group all simple fixed factor Laplacians under appropriate base class

bf9cd3c

Change all test data from uniform cell areas to random cell areas

0ee4277

- This is a more realistic and stronger test

Introduce separate pytest fixture for tripolar grids

0cf7dc8

Update tutorial.ipynb

6c79f21

* Pass REGULAR and REGULAR_WITH_LAND Laplacian the additional grid variable "area" * Fix typos and implement Scott's comments

Merge branch 'master' of https://github.com/ocean-eddy-cpt/gcm-filters …

112342a

…into fixed-factor-filtering

Update user warning in filter class

9a9fed7

Add test that checks that dx_min warning is raised

8d70243

Merge branch 'fixed-factor-filtering' of github.com:NoraLoose/gcm-fil…

f6bcec7

…ters into fixed-factor-filtering

Merge branch 'fixed-factor-filtering' of github.com:NoraLoose/gcm-fil…

5cb08da

…ters into fixed-factor-filtering

Modify filter warn message

1c93f46

Merge branch 'fixed-factor-filtering' of github.com:NoraLoose/gcm-fil…

9039404

…ters into fixed-factor-filtering

Format warnings properly

26c7800

Rerun tutorial.ipynb to get updated warnings

a675207

Update GPU tutorial with internal fixed factor filtering

3afd84c

Update tutorial_filter_types.ipynb: simple fixed factor

72bffc8

Update simple fixed factor filtering in tripole tutorial

4b99312

Make pre-commit happy

ece6a31

NoraLoose linked an issue Jun 11, 2021 that may be closed by this pull request

Handle simple fixed factor filtering internally #71

Closed

NoraLoose added 4 commits June 11, 2021 18:00

Fix more typos in tutorials

5e5004a

Update simple fixed factor in numerical instab. tutorial

6ad40ab

laplacian --> Laplacian

726e264

Implement Scott's comments into Filter Theory

315fd50

Merge branch 'fixed-factor-filtering' of github.com:NoraLoose/gcm-fil…

82023b1

…ters into fixed-factor-filtering

iangrooms approved these changes Jun 12, 2021

View reviewed changes

NoraLoose added 2 commits July 8, 2021 12:31

Update all tutorials according to new Laplacian naming conventions

deb4d85

NoraLoose mentioned this pull request Jul 9, 2021

Docs update #78

Merged

rabernat reviewed Jul 27, 2021

View reviewed changes

NoraLoose added 3 commits July 27, 2021 12:09

Raise ValueError rather than warning ...

8b5c461

* ... if dx_min is not equal to 1 for simple fixed factor filtering * update test accordingly

Link to documentation page for numerical instability warning

842bfe2

NoraLoose added 2 commits July 27, 2021 12:41

Fix small typo in docstring

4dbdda0

Remove print statements that I used for debugging

06fb99a

NoraLoose mentioned this pull request Jul 27, 2021

Refactor kernel tests #79

Merged

3 tasks

rabernat reviewed Jul 28, 2021

View reviewed changes

NoraLoose added 2 commits July 28, 2021 08:34

Remove line that will never run

9072e76

Rename kernels and improve docstrings

e2ab6c9

* change Laplacian naming convention from TRANSFORMED_TO to AREA_WEIGHTED * update tests according to new naming convention * reformat docstrings in kernel.py module so lists show up properly in API * link docstrings to kernel methods

rabernat merged commit 05b58b9 into ocean-eddy-cpt:master Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle simple fixed factor filtering internally #75

Handle simple fixed factor filtering internally #75

NoraLoose commented Jun 11, 2021 •

edited

Loading

review-notebook-app bot commented Jun 11, 2021

codecov-commenter commented Jun 11, 2021 •

edited

Loading

iangrooms left a comment

rabernat left a comment

rabernat Jul 27, 2021

rabernat Jul 27, 2021

rabernat Jul 27, 2021

rabernat Jul 27, 2021

NoraLoose commented Jul 27, 2021

rabernat left a comment •

edited

Loading

rabernat Jul 28, 2021

NoraLoose Jul 28, 2021

rabernat Jul 29, 2021

rabernat Jul 28, 2021

rabernat Jul 28, 2021

NoraLoose Jul 28, 2021

rabernat Jul 29, 2021

rabernat Jul 28, 2021

NoraLoose Jul 28, 2021

rabernat Jul 28, 2021

NoraLoose commented Jul 28, 2021 •

edited

Loading

rabernat commented Jul 29, 2021

		pass


		ALL_KERNELS[GridType.TRANSFORMED_TO_REGULAR] = RegularLaplacianWithArea



		ALL_KERNELS[
		GridType.TRANSFORMED_TO_REGULAR_WITH_LAND

Handle simple fixed factor filtering internally #75

Handle simple fixed factor filtering internally #75

Conversation

NoraLoose commented Jun 11, 2021 • edited Loading

Issue

Changes

Tutorial changes

review-notebook-app bot commented Jun 11, 2021

codecov-commenter commented Jun 11, 2021 • edited Loading

Codecov Report

iangrooms left a comment

Choose a reason for hiding this comment

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

NoraLoose commented Jul 27, 2021

rabernat left a comment • edited Loading

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

Choose a reason for hiding this comment

NoraLoose commented Jul 28, 2021 • edited Loading

rabernat commented Jul 29, 2021

NoraLoose commented Jun 11, 2021 •

edited

Loading

codecov-commenter commented Jun 11, 2021 •

edited

Loading

rabernat left a comment •

edited

Loading

NoraLoose commented Jul 28, 2021 •

edited

Loading