Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Updating filterSpec #20

Closed
iangrooms opened this issue Feb 1, 2021 · 4 comments
Closed

Updating filterSpec #20

iangrooms opened this issue Feb 1, 2021 · 4 comments

Comments

@iangrooms
Copy link
Member

I've created a branch where I updated filter.py in several ways:

  1. Added a default calculation of N based on the coarsening factor (filter scale vs grid scale). Default value results in errors between .001 and .01 in the polynomial approximation of the target filter.
  2. Added an optional argument d for the dimension of the grid. @jakesteinberg is using 1D data, and it helps to have an option for 1d instead of 2d.
  3. Changed the meaning of Lf for the Gaussian filter so that it's now Lf=standard devation of Gaussian kernel. (Changed by a factor of root 2.)
  4. Changed the meaning of Lf for the Taper filter so that it's now 2*pi/Lf is the wavenumber set to zero. (Changed by a factor of 2.)

What do you think about these changes? We have a recipe for connecting the "target" grid scale to Lf. For the "Taper" filter Lf is now the same as the target grid scale, but for the Gaussian the target grid scale is \sqrt{12}*Lf.

@rabernat
Copy link
Collaborator

rabernat commented Feb 2, 2021

I really like these changes. They make the filter calculation more user friendly.

I would like to note that I am basically maintaining a mirror of this code here: https://github.com/ocean-eddy-cpt/gcm-filters/blob/master/gcm_filters/filter.py

In particular, I have renamed some of the input arguments to be more verbose and follow standard python naming conventions. I've also attempted to document them to the best of my understanding:
https://github.com/ocean-eddy-cpt/gcm-filters/blob/04ad83562a670db8d194d7ad672059d74197a109/gcm_filters/filter.py#L157-L175

My hope was that the actual code for the filters could live in the package. The package is basically ready for use and testing and only lacks kernels (see ocean-eddy-cpt/gcm-filters#8, ocean-eddy-cpt/gcm-filters#9, ocean-eddy-cpt/gcm-filters#10).

With your new changes to filterSpec in this repo, much of that is now out of date. If the filterSpec code in this repo (gcm-filters-paper) continues to change, it will be a lot of repetitive work to keep them in sync. So I see two ways forward:

  1. We stop working on the gcm-filters package until the details here converge more
  2. The people working on this paper start using the gcm-filters package for the analysis in this paper and contribute any further changes as pull requests over there

I'm open to whatever you think is best. I just want to communicate clearly to avoid further inefficient duplication between the two repos.

@iangrooms
Copy link
Member Author

I'm in favor of moving development of the filtering code over to the gcm-filters repo. But I'm not actually using the code, so I will defer to @NoraLoose, @jakesteinberg, and @ElizabethYankovsky.

@NoraLoose
Copy link
Member

Moving the code development over to the gcm-filters repo sounds good to me, too. I'm hoping to get to some of the kernel issues (e.g., ocean-eddy-cpt/gcm-filters#9) next week. @iangrooms, are you going to submit a pull request over there with your changes to filterSpec?

@rabernat
Copy link
Collaborator

rabernat commented Feb 2, 2021

Let me know how I can help with this. The documentation for the package has not really been written, making it hard to know where to start. I can try to do some of that today.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants