search

ocean-eddy-cpt / gcm-filters

Diffusion-based Spatial Filtering of Gridded Data
https://gcm-filters.readthedocs.io/
Other
37 stars 24 forks source link

Documentation of filter types #68

Closed NoraLoose closed 3 years ago

NoraLoose commented 3 years ago

This adds a new tutorial which extends and updates tutorial_irregular_grid.ipynb by several aspects:

The tutorial is dependent on https://github.com/ocean-eddy-cpt/gcm-filters/pull/28 being merged since it uses the kappa's for anisotropic and spatially-varying filtering.

I had to comment out the check for large files in the pre-commit config to be able to submit this new tutorial. Is there a better solution?

review-notebook-app[bot] commented 3 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov-commenter commented 3 years ago

Codecov Report

Merging #68 (3274d67) into master (641e010) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   98.47%   98.47%           
=======================================
  Files           7        7           
  Lines         719      719           
=======================================
  Hits          708      708           
  Misses         11       11
Flag Coverage Δ
unittests 98.47% <ø> (ø)

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

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 641e010...3274d67. Read the comment docs.

NoraLoose commented 3 years ago

Thanks for looking through the tutorial, @iangrooms! I fixed the typo.

But I think it would be pretty hard to pick a single stage order that would somehow strike a balance for good stability over the whole domain, so I don't think we need to change anything.

Agreed. :+1:

rabernat commented 3 years ago

More great work. Thanks for this Nora!

(For some reason, reviewNB is not able to post my comments, so posting them here directly.)

Data

Is the catalog.yaml file public? I think not. So how will a user be able to run this notebook?

I propose we make the cpt-data repo public. And point directly at the catalog url here: https://raw.githubusercontent.com/ocean-eddy-cpt/cpt-data/master/catalog.yaml.

"Simple Fixed Factor Filter"

We might want to consider implementing this internally within gcm-filters rather than requiring the user to jump through these hoops manually..

NoraLoose commented 3 years ago

Thanks @rabernat for your comments!

Is the catalog.yaml file public?

I copied it over from the cpt-data repo to the gcm-filters repo (into the docs folder), but making the cpt-data repo public would probably be cleaner.

We might want to consider implementing this internally within gcm-filters rather than requiring the user to jump through these hoops manually..

If we want to implement this internally, this would have to happen in the filter.py module (rather than the kernels.py module) because

We would want to do this only for a subset of Laplacians: REGULAR, REGULAR_WITH_LAND, TRIPOLAR_REGULAR_WITH_LAND, and all of these would need the additional required grid variable area.

Are there any disadvantages of implementing this, e.g., can we imagine situations where the user wants to use the REGULAR Laplacian without the area-weighting? I can't think of any, but maybe someone else can?

iangrooms commented 3 years ago

I think if the grid is really regular, then there's not much need to use gcm-filters, so it seems like a good idea to me to make REGULAR into simple-fixed-factor.

NoraLoose commented 3 years ago

The only remaining issue with this tutorial is that simple fixed factor filtering should be done internally by gcm-filters. I opened issue #71, but may not get to implementing/solving it right now. Do we want to merge this tutorial in the meantime, and update it once #71 is solved?

iangrooms commented 3 years ago

Do we want to merge this tutorial in the meantime, and update it once #71 is solved?

That makes sense to me. Use different PRs for different problems: Add tutorial with this PR; updated REGULAR/simple fixed factor with a separate PR.