Add apply_mask function to mask sub-package
#905
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and allow it to handle a DataArray input, modify existing tests for validate_source_ds
…s for the core method in apply_mask
… add associated test in test_apply_mask
… provided as a path
…ue)=DataArray conflict with xr.where
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
leewujung
reviewed
Dec 20, 2022
Comment on lines
+278
to
+307
| @pytest.mark.parametrize( | ||
| ("n", "n_chan", "var_name", "mask", "mask_file", "fill_value", "is_delayed", "var_masked_truth"), | ||
| [ | ||
| (2, 1, "var1", np.identity(2), None, np.nan, False, np.array([[1, np.nan], [np.nan, 1]])), | ||
| (2, 1, "var1", np.identity(2), None, 2.0, False, np.array([[1, 2.0], [2.0, 1]])), | ||
| (2, 1, "var1", np.identity(2), None, np.array([[[np.nan, np.nan], [np.nan, np.nan]]]), | ||
| False, np.array([[1, np.nan], [np.nan, 1]])), | ||
| (2, 1, "var1", np.identity(2), None, xr.DataArray(data=np.array([[[np.nan, np.nan], [np.nan, np.nan]]]), | ||
| coords={"channel": ["chan1"], | ||
| "ping_time": [0, 1], | ||
| "range_sample": [0, 1]}), | ||
| False, np.array([[1, np.nan], [np.nan, 1]])), | ||
| (2, 1, "var1", [np.identity(2), np.array([[0, 1], [0, 1]])], [None, None], 2.0, | ||
| False, np.array([[2.0, 2.0], [2.0, 1]])), | ||
| (2, 1, "var1", np.identity(2), None, 2.0, True, np.array([[1, 2.0], [2.0, 1]])), | ||
| (2, 1, "var1", np.identity(2), "test.zarr", 2.0, True, np.array([[1, 2.0], [2.0, 1]])), | ||
| (2, 1, "var1", [np.identity(2), np.array([[0, 1], [0, 1]])], ["test0.zarr", "test1.zarr"], 2.0, | ||
| False, np.array([[2.0, 2.0], [2.0, 1]])), | ||
| (2, 1, "var1", [np.identity(2), np.array([[0, 1], [0, 1]])], ["test0.zarr", None], 2.0, | ||
| False, np.array([[2.0, 2.0], [2.0, 1]])), | ||
| ], | ||
| ids=["single_mask_default_fill", "single_mask_float_fill", "single_mask_np_array_fill", | ||
| "single_mask_DataArray_fill", "list_mask_all_np", "single_mask_ds_delayed", | ||
| "single_mask_as_path", "list_mask_all_path", "list_mask_some_path"] | ||
| ) | ||
| def test_apply_mask(n: int, n_chan: int, var_name: str, | ||
| mask: Union[np.ndarray, List[np.ndarray]], | ||
| mask_file: Optional[Union[str, List[str]]], | ||
| fill_value: Union[int, float, np.ndarray, xr.DataArray], | ||
| is_delayed: bool, var_masked_truth: np.ndarray): |
There was a problem hiding this comment.
I think what test contains right now is more of the integrated test flavor, and some of those can be split out to unit test the mask validation and collection function.
My suggestion is create/split things into the following:
- unit tests for
validate_and_collect_mask_input:- test allowable types of mask sources
- test the allowable forms of mask (list or single)
- unit tests for
_check_var_name_fill_value- test whether
var_nameis insource_ds - test whether
fill_valueis of the same shape assource_ds["var_name"]
- test whether
- integration tests that actually applies the tests (you have these already)
leewujung
reviewed
Dec 20, 2022
leewujung
requested changes
Dec 20, 2022
There was a problem hiding this comment.
@b-reyes : thanks for the PR! All of my actual comments are textual, with the exception of the tests -- I think it would be good to set up unit tests for the functions used by the main function.
Update: I think we should consider the suggested test refactoring as a separate PR as part of the test hackday, so that this can be merged by end of day for @emiliom to add the provenance piece.
b-reyes
commented
Dec 20, 2022
b-reyes
commented
Dec 20, 2022
b-reyes
commented
Dec 20, 2022
Co-authored-by: Wu-Jung Lee <leewujung@gmail.com>
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## dev #905 +/- ##
==========================================
- Coverage 78.71% 72.46% -6.26%
==========================================
Files 56 5 -51
Lines 5253 345 -4908
==========================================
- Hits 4135 250 -3885
+ Misses 1118 95 -1023
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@leewujung I have addressed all of your comments. As you suggested, we will wait to do the test refactoring as a separate PR as part of the test hackday. |
leewujung
added a commit
that referenced
this pull request
Dec 22, 2022
See #905 for all conversations and detailed commits.
lsetiawan
pushed a commit
to lsetiawan/echopype
that referenced
this pull request
Feb 27, 2023
…cs#912) See OSOceanAcoustics#905 for all conversations and detailed commits.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the
apply_maskfunction discussed in #817 (comment) and subsequent comments in #817.