Handling of provenance attributes in apply-mask and add-depth, especially for testing

[emiliom]

Addresses errors in test_mask.py::test_apply_mask discussed in #926 (comment). These errors stemmed from the addition of provenance attributes (PR #918). The solution at this time was to avoid making dataarray comparisons that also compare attributes, replacing dataarray.identical with dataarray.equals.

Also modified mask.api.frequency_differencing to ensure chanA & chanB in are set to string type; and mask.api.apply_mask to test for dask array in addition to xr array (in _variable_prov_attrs), and to specify in the docstring that an Sv variable is expected.

After #929, the consolidate tests are now running by default. This exposed an error in test_consolidate.py::test_add_depth of the same type as the attribute testing error in test_mask.py::test_apply_mask. I "fixed" it by commenting out the assert statement that specifically tested for attributes.

[b-reyes]

@emiliom a couple of comments:

• From my understanding, we intended for apply_mask to be very general and this is why var_name was added. It seems like we should instead change the code https://github.com/OSOceanAcoustics/echopype/blob/dev/echopype/mask/api.py#L244, to allow for this generality.
• Also, could we move the function _variable_prov_attrs outside of apply_mask? I feel like this decreases the readability of the code. We could do this in this PR or another one.

[emiliom]

From my understanding, we intended for apply_mask to be very general and this is why var_name was added. It seems like we should instead change the code https://github.com/OSOceanAcoustics/echopype/blob/dev/echopype/mask/api.py#L244, to allow for this generality.

To be clear, what I meant elsewhere as "general" or "generic" was that the docstring did not clearly say that an Sv type of variable was the target for var_name. var_name just provides the flexibility for the variable (dataarray) name to be anything. Based on our discussion elsewhere, I made small edits to the docstring to make it clear that an Sv variable is expected. Do the edits look good?

Also, could we move the function _variable_prov_attrs outside of apply_mask? I feel like this decreases the readability of the code. We could do this in this PR or another one.

I like that, thanks. I'll push a commit to this PR.

[b-reyes]

To be clear, what I meant elsewhere as "general" or "generic" was that the docstring did not clearly say that an Sv type of variable was the target for var_name. var_name just provides the flexibility for the variable (dataarray) name to be anything. Based on our discussion elsewhere, I made small edits to the docstring to make it clear that an Sv variable is expected. Do the edits look good?

@emiliom The original code (i.e. that produced in #905) was created such that it does not expect var_name to be Sv (and so this should not be provided in the docstring). I believe this generality could be useful down the road if we would like to use it for purposes other than to apply a mask to Sv data. Thus, I suggest that we modify the attributes added in #918 so that we do not specify Sv. Is this possible? Perhaps I am missing something critical here that does not allow for this generality to be achieved.

Also, could we move the function _variable_prov_attrs outside of apply_mask? I feel like this decreases the readability of the code. We could do this in this PR or another one.

I like that, thanks. I'll push a commit to this PR.

Great!

[leewujung]

var_name just provides the flexibility for the variable (dataarray) name to be anything. Based on our discussion elsewhere, I made small edits to the docstring to make it clear that an Sv variable is expected. Do the edits look good?

@emiliom: I think the edits you made captures the intended case captures what I intended, so let's roll with it.

I noted above a commented out assertion -- see how you want to handle it, but it doesn't seem a good idea to just not test for anything.

Otherwise, feel free to merge this PR. Thanks!

[codecov-commenter]

Codecov Report

Merging #930 (6407959) into dev (9e19736) will decrease coverage by 1.93%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##              dev     #930      +/-   ##
==========================================
- Coverage   79.16%   77.22%   -1.94%     
==========================================
  Files          58        5      -53     
  Lines        5504      303    -5201     
==========================================
- Hits         4357      234    -4123     
+ Misses       1147       69    -1078     

Flag	Coverage Δ
unittests	77.22% <85.71%> (-1.94%)	⬇️

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

Impacted Files	Coverage Δ
echopype/mask/api.py	82.40% <80.00%> (-0.94%)	⬇️
echopype/consolidate/api.py	69.69% <100.00%> (-18.83%)	⬇️
echopype/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
echopype/consolidate/split_beam_angle.py	76.85% <0.00%> (-11.12%)	⬇️
echopype/calibrate/api.py
echopype/calibrate/calibrate_azfp.py
echopype/calibrate/calibrate_base.py
echopype/calibrate/calibrate_ek.py
echopype/calibrate/env_params.py
... and 48 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[emiliom]

Also, could we move the function _variable_prov_attrs outside of apply_mask? I feel like this decreases the readability of the code. We could do this in this PR or another one.

Done. In doing so, I added type hints for _variable_prov_attrs, and that led me to remove type checking for dask.array.Array since that wasn't present in the apply_mask type hints. I also simplified the type hint for mask in apply_mask, to remove an unnecessary Union grouping.

[emiliom]

Thanks @leewujung . See my comments. I prefer to leave the test_consolidate.py::test_add_depth attribute assertion test commented out for now, for this release. We'll revisit this in the next release.

I'll wait for you to take another look before merging this PR.

[emiliom]

Note about the pre-commit.ci build error: The error involves the Jupyter Book build:

build of https://github.com/executablebooks/jupyter-book@v0.12.3 for python@python3 exceeds tier max size 250MiB: 272MiB

I don't know if this is a new, lower limit or if it's been happening if the error has been happening for a while. But it's not related to this PR per se.

[leewujung]

The intent is to handle variable attribute testing more systematically in the next release.

Sounds good -- let's make sure to follow through on this! Feel free to merge.

For the Jupyter Book build error: yes it is in all PRs, but I haven't had a chance to look into what that is. Perhaps @lsetiawan has some insight here?

[emiliom]

Thanks! See the new issue I've created, #933, to track attribute testing.

I'll go ahead and merge the PR