AZFP parser error catch to handle case of raw file with no temperature or tilt

[emiliom]

Addresses remaining issue in #198, where an AZFP raw file without temperature data leads to a failure during open_raw. As diagnosed there, the failure comes about in parser_azfp.compute_temp due to invalid R values passed to math.log. A test has been added that will catch cases of the R denominator being <= 0 and return np.nan instead.

This has been tested with an AZFP file (and corresponding XML) provided by @dsmossman.

Remaining tasks:

• Add AZFP test files to our test data
• Add a test

[emiliom]

This PR will be ready for review once all the CI tests pass.

[leewujung]

@emiliom : I have a suggestion re the statement you used to handle the temperature conversion error.

Also, while you are on it, I think we should change the tilt calculations too. All tilt-related parameters in self.parameters are also all 0 in the case of the glider, likely due to the same reason that the glider is recording the attitude separately just like the temperature. I suggest that in compute_tilt we check for the parameter values, and if they are all 0, return NaN (instead of 0 as it currently returns), so that users do not think that they are actually getting tilt=0 data.

[emiliom]

Also, while you are on it, I think we should change the tilt calculations too. All tilt-related parameters in self.parameters are also all 0 in the case of the glider, likely due to the same reason that the glider is recording the attitude separately just like the temperature. I suggest that in compute_tilt we check for the parameter values, and if they are all 0, return NaN (instead of 0 as it currently returns), so that users do not think that they are actually getting tilt=0 data.

Makes sense. I'll take a closer look. I think it'll be more efficient, computationally, to do these tests outside the compute_temp and compute_tilt functions. I'll submit a commit with an implementation that you can review.

[leewujung]

As an overall comment: I am about to write AZFP an email so I'll include the questions about whether these values are always zero in it!

[emiliom]

New commits:

• Refactored the compute_temp test for invalid (= 0) temperature parameters.
• Added compute_tilt test for invalid (= 0) tilt parameters.

For both tests, I set the testing of actual parameter values outside the compute_* functions, so they happen only once. The functions are then passed a simple boolean.

Also, in the test module I replaced the use of joinpath with pathlib path concatenation.

[codecov-commenter]

Codecov Report

Merging #1020 (aa26e78) into dev (2736ee9) will decrease coverage by 2.82%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev    #1020      +/-   ##
==========================================
- Coverage   80.45%   77.63%   -2.82%     
==========================================
  Files          67       18      -49     
  Lines        6038     2880    -3158     
==========================================
- Hits         4858     2236    -2622     
+ Misses       1180      644     -536     

Flag	Coverage Δ
unittests	77.63% <100.00%> (-2.82%)	⬇️

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

Impacted Files	Coverage Δ
echopype/convert/parse_azfp.py	92.73% <100.00%> (+1.07%)	⬆️

... and 51 files with indirect coverage changes

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

[leewujung]

Great, thanks @emiliom! I'll comment on this once we hear from ASL and if the 0s are indeed the default behavior, we can merge this.

Also, in looking at the code, I had the urge to move the compute_* functions to be private functions outside of parse_raw to make things more readable, and some location of the constants. Do you prefer for those to be in a separate PR?

[emiliom]

Pushed new commit to add test checks for tilt_x and tilt_y similar to the one for temperature.

[emiliom]

Also, in looking at the code, I had the urge to move the compute_* functions to be private functions outside of parse_raw to make things more readable, and some location of the constants. Do you prefer for those to be in a separate PR?

You can submit a PR against my PR. I'll take a quick look before merging.

[leewujung]

@emiliom : the movements end up to be a much broader job than I originally thought (it's too fun to refactor!), so I think I'll submit a separate PR for that... I have those done (in this branch), but I'll wait to submit the PR until this one is merged, so that it's easier to manage. Seeing the TODO you have in #1023, I'll submit a PR so that the AZFP software version field can make its way in via the new structure.

[leewujung]

I haven't heard from AZFP. I think we can merge this tomorrow if I don't hear back by then, so that this and #1024 can both get merged. We have a few small fixes in v0.7.1 and it'll be good to release by end of this week so that those changes get to actual package.

[emiliom]

Per email from ASL Environmental to @leewujung, it's been confirmed that the temperature (and tilt?) parameters in the XML file are set to zero when the sensor is deactivated.

So, we can merge this PR as is, right? Then, PR #1024 can be merged, after updating it to reflect this PR.

[leewujung]

Yes, the values for temperature and tilt sensor coefficients will be 0 in glider-specific AZFP deployments.

Feel free to merge this, and then I'll update #1024 and merge that too!