Adding default n_steps and adding ndim

[iangrooms]

This PR does a few things:

1. Add in a default for n_steps that depends on the coarsening factor (filter_scale/dx_min)
2. Add an option to switch between 1d and 2d grids ndmin
3. Change the meaning of filter_scale in both filter shapes, so that it always corresponds to a boxcar of width filter_scale

[rabernat]

Thanks for getting this started @iangrooms! Let me know when you're ready for some feedback.

The "pre-commit" part of the checks does code linting (checking that the code formatting conforms to standards, etc.) You can set it up locally following the contributor guide and have it run automatically every time you commit. It will give hints on how to satisfy its checks.

[iangrooms]

@rabernat this pull request should be ready. Not sure why it says it's failing the pre-commit, since it passes on my machine. Also, all tests currently pass.

[rabernat]

This is great @iangrooms! Congrats on successfully jumping through all of the hoops (tests, pre-commit, etc.)

I don't understand the pre-commit failure either, but for now I think we can safely ignore it.

I have a few tiny suggestions for docstring formatting and a question about how to handle the ndim parameter.

[codecov-io]

Codecov Report

Merging #20 (8777072) into master (2dded95) will decrease coverage by 1.69%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
- Coverage   97.64%   95.94%   -1.70%     
==========================================
  Files           7        7              
  Lines         212      222      +10     
==========================================
+ Hits          207      213       +6     
- Misses          5        9       +4     

Flag	Coverage Δ
unittests	95.94% <72.22%> (-1.70%)	⬇️

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

Impacted Files	Coverage Δ
tests/test_filter.py	100.00% <ø> (ø)
gcm_filters/filter.py	95.28% <70.58%> (-3.68%)	⬇️
gcm_filters/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 2dded95...8777072. Read the comment docs.

[rabernat]

Ok great!

Do you see anything in the tutorial that needs to be updated because of the API change? If not, I think this is ready to merge.

[iangrooms]

Do you see anything in the tutorial that needs to be updated because of the API change? If not, I think this is ready to merge.

I checked the tutorial, and there shouldn't be any changes. I also added an error if the user tries to use ndim>2 with the default n_steps. So I think it should be ready

[rabernat]

Ok great, I'll merge. Thanks a lot @iangrooms!

For reference, the coverage report is referring to the fact that some parts of the new if / else block for ndim are not covered by tests (red lines here): https://codecov.io/gh/ocean-eddy-cpt/gcm-filters/pull/20/src/gcm_filters/filter.py?before=gcm_filters/filter.py#L70...82

To keep test coverage high, technically we should add test scenarios for each of these different options, including the errors. However, that feels a little too strict for the early stage this package is at, so we will let it slide for now. I'll put a reminder in #18 about this.

[rabernat]

@jbusecke it won't let me merge because of the pre-commit failure. Can you look at this and try to figure out what's going on? Ian says the pre-commit passes locally, but the workflow is still failing. If the pre-commit is going to be flaky like this, I feel like we should disable it as a required check.

[jbusecke]

Let me look into this.

[jbusecke]

I can reproduce the error locally on my machine. @iangrooms could you confirm which conda environment you used? Did you run pre-commit run --all-files from the root directory?

Another thing I noticed is that you started this PR from your master branch. Not sure if that is causing this trouble though. In general it is recommended to check out a new branch with git checkout -b <new_cool_branchname>.

[jbusecke]

Oh I see this was solved. What was the culprit in the end?

[iangrooms]

pre-commit changes files, including ones that I wasn't working on (kernels.py). So when I ran pre-commit it would fail, then I'd make some changes, then it would pass. But I didn't commit the changes that pre-commit made to files that I wasn't working on. That's why pre-commit passed locally but failed on the server; once I committed the changes to kernels.py (not made by me), it fixed the problem on the server.

[rabernat]

once I committed the changes to kernels.py (not made by me), it fixed the problem on the server.

Yet strangely, kernels.py does not show up in the files changed tab of this PR. 🧐

I'm still learning about pre-commit myself, so it's useful to get this feedback.

[jbusecke]

Thanks for the feedback @iangrooms. That is quite strange, actually. It should not let you push the commit if there are problems with the committed files. Or at least that is how I understand it.

For what its worth, I personally always do a manual pre-commit run pre-commit run --all-files, then another git add -u before the commit I want to push, but that is maybe not the smoothest way of doing it.