Conversation
This comment has been minimized.
This comment has been minimized.
to indicate when diff_freq on load has been done
singletons should be compared with is, not ==
no implementation yet--test driven development
sky_subtract.py in diff_freq
no-lex
force-pushed
the
extra_keywords
branch
from
69546f5 to
689ff30
Compare
|
Rebased over |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 857 904 +47
=========================================
+ Hits 857 904 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # Check that the weights summed correctly | ||
| assert np.all(test_weights == ins.weights_array), "Weights did not sum properly" | ||
|
|
||
| def test_extra_keywords(): |
There was a problem hiding this comment.
This test doesn't appear to check whether diff_freq is set, etc.
| ins.metric_ms = ins.mean_subtract() | ||
| assert np.all(ins.metric_ms.mask), "The metric_ms array was not all masked" | ||
|
|
||
| def test_flag_uvf_freq(): |
There was a problem hiding this comment.
This test checks the error handling, but I don't see that it ever checks that the answer is right when the right calls are made.
| assert np.all(combo_ins.metric_array.data == truth_ins.metric_array.data) | ||
| assert np.all(combo_ins.metric_array.mask == truth_ins.metric_array.mask) | ||
|
|
||
| def test_mask_to_flags_2(): |
There was a problem hiding this comment.
This test also does not check anything other than that the code runs through. I think the smaller test files should support a "gets the correct answer" test. If not, you can use "select-on-read" functionality to make a testable object where it is not too hard to calculate the answer ahead of time through alternate means (you could even modify the starting data array to make this easy).
| ss.read(testfile, read_data=True, diff=False, diff_freq=False) | ||
| ss.apply_flags(flag_choice='original') | ||
| assert ss.flag_array is not None | ||
| temp_array = np.logical_or(ss.flag_array, ss.flag_array) |
There was a problem hiding this comment.
This line and those below confuse me. This line should just return a copy of ss.flag_array, since ORing an array with itself will just return an array with 1 everywhere that the original array was nonzero and 0 otherwise, and the flag array is already boolean. Due to this, the next two lines are guaranteed to satisfy the assertion, since ss.flag_array[::2] is now just a subarray of temp_array. Maybe you meant to index the ss.flag_array's being fed into the np.logical_or call, e.g. `np.logical_or(ss.flag_array[:, :-1], ss.flag_array[:, 1:])?
4 participants
This pull request adds a
dif_freqkeyword and frequency differencing mode (the natural compliment to the time differencing mode defaulted to), and this mode is properly represented between INS and SS objects. For backwards compatibility, the time differencing is kept asdiffwhile thediff_freqis indicated for frequency differencing.