Rect 2d filter fit basis #96
Conversation
aewallwi
force-pushed
the
rect_2d_filter_fit_basis
branch
from
b1a9f54 to
9e565d8
Compare
aewallwi
force-pushed
the
rect_2d_filter_fit_basis
branch
from
9e565d8 to
0e6977b
Compare
There was a problem hiding this comment.
Overall this looks like a great addition, though I have left some comments that I would like addressed before approving. In particular, I would like the inconsistency between the docstrings for the filt2d_mode options and the value checking done in _clean_filter to be sorted out before approving.
Thanks for your work and the nice addition!
| @@ -477,6 +498,7 @@ def fourier_filter(x, data, wgts, filter_centers, filter_half_widths, mode, | |||
| filter_dims_d = [1] | |||
| suppression_factors = filter_kwargs.pop('suppression_factors') | |||
| max_contiguous_edge_flags = filter_kwargs.pop('max_contiguous_edge_flags') | |||
| filt2d_mode = filter_kwargs.pop('filt2d_mode') | |||
There was a problem hiding this comment.
filt2d_mode is popped from filter_kwargs here, but it doesn't look like it's passed to _fit_basis_2d—is this intentional?
| # determine rectangular region by taking outer products of filter areas along fourier time and fourier freq | ||
| # axes. | ||
| area_t = np.outer(_get_filter_area(x[0], fc_t, fw_t), _get_filter_area(x[1], fc_f, fw_f)) | ||
| elif filt2d_mode == 'plus': |
There was a problem hiding this comment.
Inconsistency here with what's written in the docstring: docstring says that supported modes are 'rect' and 'outer', but this uses 'rect' and 'plus'.
| av = _get_filter_area(x[0], fc_t, fw_t) | ||
| area_t[:, np.argmin(np.abs(_x[1]-fc_f))] = av | ||
| else: | ||
| raise ValueError("%s is not a valid filt2d_mode! choose from ['rect', 'plus']"%(filt2d_mode)) |
There was a problem hiding this comment.
Can write this a bit more compactly using an f-string: f"{filt2d_mode} is not a valid filt2_mode! ..."
| wgts_time[i] = 0. | ||
| for i, _y, _w, in zip(range(model.shape[1]), model.T, wgts_time.T): | ||
| if filt2d_mode == 'outer' or not filter2d: | ||
| for i, _y, _w, in zip(range(data.shape[0]), data, wgts): |
There was a problem hiding this comment.
An explanatory comment would be helpful here (it's a little hard to parse whether the diff makes sense without knowing what this is meant to do).
| model += model_temp | ||
| # repeate this for all the rectangle bounds provided. | ||
| else: | ||
| raise ValueError("%s not a supported filt2d_mode ('rect', 'outer')"%filt2d_mode) |
There was a problem hiding this comment.
Another place where an f-string could be used instead of the old style string formatting.
| nt.assert_true(np.all(np.isclose(mdl1, mdl2))) | ||
| nt.assert_true(np.all(np.isclose(res1, res2))) | ||
| # assert ValueError for unsupported filt2d_mode | ||
| nt.assert_raises(ValueError, dspec.fourier_filter, x=[times, freqs], data=d, wgts=w, filter_centers=[[0.],[0.]], |
There was a problem hiding this comment.
If possible, it would be good to run similar tests with the other filtering modes. In particular, I would expect this same set of tests to fail if it were run using something that calls _clean_filter. On that note, it looks like only the 'clean' mode will call this, but 'dayenu_clean' will not—is that intended?
| dpss_options1_2d = {'eigenval_cutoff': [[1e-12], [1e-12]], 'filt2d_mode': 'rect'} | ||
| dpss_options2_2d = {'eigenval_cutoff': [[1e-12], [1e-12]], 'filt2d_mode': 'outer'} | ||
| dpss_options3_2d = {'eigenval_cutoff': [[1e-12], [1e-12]], 'filt2d_mode': 'plus'} | ||
| mdl1, res1, info1 = dspec.fourier_filter(x=[times, freqs], data=d, wgts=w, filter_centers=[[0.],[0.]], |
There was a problem hiding this comment.
I'm not super familiar with what to expect from this, but it seems like choosing the origin as the filter center could be too simple of a test? At the very least, it would be nice to see that this works for any choice of filter center. (I trust that you've convinced yourself that it will work, but it would be reassuring to see this be tested at a non-trivial choice of filter center if possible.)
| @@ -890,6 +890,24 @@ def get_snr(clean, fftax=1, avgax=0, modes=[2, 20]): | |||
| nt.assert_raises(ValueError, dspec.fourier_filter,x=flog, data=d, wgts=w, filter_centers=[0.], | |||
| filter_half_widths=[bl_len], | |||
| mode='clean', filter_dims=[1], **{'tol':1e-5}) | |||
|
|
|||
| # check that 2d clean with single rectangular region is the same for outer and rect filter2d modes. | |||
There was a problem hiding this comment.
Can you make this its own test? I think best practice is to use directed, atomic tests with as few failure points as possible, rather than large tests that have many failure points.
closes #95