Raise error message if filter_scale < dx_min

[NoraLoose]

No description provided.

[codecov-io]

Codecov Report

Merging #25 (9865665) into master (8b92035) will decrease coverage by 0.40%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   95.55%   95.15%   -0.41%     
==========================================
  Files           7        7              
  Lines         225      227       +2     
==========================================
+ Hits          215      216       +1     
- Misses         10       11       +1     

Flag	Coverage Δ
unittests	95.15% <33.33%> (-0.41%)	⬇️

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

Impacted Files	Coverage Δ
gcm_filters/filter.py	93.63% <33.33%> (-0.81%)	⬇️

---

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 8b92035...9865665. Read the comment docs.

[rabernat]

For a change like this, it would be great to add a test that covers the error, similar to this:

gcm-filters/tests/test_filter.py

Lines 102 to 108 in 8b92035

	# check that we get an error if we leave out any required grid_vars
	for gv in grid_vars:
	grid_vars_missing = {k: v for k, v in grid_vars.items() if k != gv}
	with pytest.raises(ValueError, match=r"Provided `grid_vars` .*"):
	filter = Filter(
	grid_type=grid_type, grid_vars=grid_vars_missing, **filter_args
	)

That will help keep the code coverage high.

It could live in a new standalone test, e.g. test_filter_errors().

[NoraLoose]

Thanks @rabernat, this makes sense.

Before adding such a test, I would like to discuss an alternative option to handle dx_min (which would require different testing).

The idea is to remove dx_min from the filter arguments, and instead

• compute dx_min in a separate function inside filter.py, if the user wants to filter with fixed filter length scale. dx_min would then be computed as the minimum grid spacing, inferred from the grid variables that the user puts in;
• set dx_min to 1, if the user wants to filter with fixed coarsening factor.

This could avoid errors and confusion on the user side. For instance, I anticipate that users will try to set dx_min = 1, even though they want to filter with fixed filter length scale. (I think @arthurBarthe ran into this issue at first.) If we decide to go this way, we should probably add a filter argument, via which the user picks a "method": fixed filter length scale or fixed coarsening factor. What do you think about this? I'm tagging @iangrooms here, since he has written most of the FilterSpec class.

[rabernat]

inferred from the grid variables that the user puts in

In general, I like the idea of inferring as much as possible from the inputs. However, it also means more work for gcm-filters to understand the meaning of all the different grid variables. So this question is closely related to the discussion in #23 (comment).