Modify combine_echodata check for reversed time coordinates

[b-reyes]

While looking into ep.combine_echodata, I noticed that we do not test for reversed time coordinates for all sensors and all times. This PR solves this. Specifically, the following items were completed.

• For EK80, EK60, and AZFP I make it so that we check for reversed time coordinates for all groups (besides Top-level, Sonar, Provenance, and Platform/NMEA) and correct them if necessary.
• Additionally, I made slight modifications to the concat_dims in core.py for each sensor. While creating consistent time coordinates, these were not modified, but they should have been. The modifications include:
  • For AZFP
    • added time2 to the platform key
  • For EK60/ES70
    • changed ping_time to time2 for the platform key
    • added time3 to the platform key
  • For EK80/ES80/EA640
    • added time3 to the platform key

[codecov-commenter]

Codecov Report

Merging #689 (eaea5d4) into dev (099b357) will decrease coverage by 51.50%.
The diff coverage is 90.47%.

@@             Coverage Diff             @@
##              dev     #689       +/-   ##
===========================================
- Coverage   79.26%   27.76%   -51.51%     
===========================================
  Files          43       41        -2     
  Lines        3888     3882        -6     
===========================================
- Hits         3082     1078     -2004     
- Misses        806     2804     +1998     

Flag	Coverage Δ
unittests	27.76% <90.47%> (-51.51%)	⬇️

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

Impacted Files	Coverage Δ
echopype/core.py	85.18% <ø> (ø)
echopype/echodata/combine.py	89.91% <90.47%> (+16.41%)	⬆️
echopype/metrics/summary_statistics.py	0.00% <0.00%> (-96.43%)	⬇️
echopype/calibrate/ecs_parser.py	0.00% <0.00%> (-95.50%)	⬇️
echopype/utils/uwa.py	6.12% <0.00%> (-87.76%)	⬇️
echopype/convert/set_groups_ek80.py	12.28% <0.00%> (-84.17%)	⬇️
echopype/convert/set_groups_ad2cp.py	16.84% <0.00%> (-82.11%)	⬇️
echopype/calibrate/calibrate_ek.py	12.45% <0.00%> (-81.03%)	⬇️
echopype/convert/parse_azfp.py	14.37% <0.00%> (-76.88%)	⬇️
echopype/convert/set_groups_ek60.py	16.66% <0.00%> (-75.44%)	⬇️
... and 23 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[leewujung]

I think the changes are ready to go once that repeated block is factored out. Thanks!

[emiliom]

Why do all these old_* and new_* variables need to be declared? I see that in check_and_correct_reversed_time, old_time is tested for None. But as far as I can see, the old_time value passed to that function will always be None, at this time. Same for new_time: the value passed to check_and_correct_reversed_time seems like it's always None at this time. The function arguments old_time and new_time could be changed from positional to keyword arguments with default None values; or removed altogether, but then you'd have tweak the function code itself.

[b-reyes]

From my understanding the code right now basically uses old_* and new_* as flags. You are right that how it is done does not make too much sense and we could probably create a better solution to this. I suggest that we open up a new issue and deal with this after 0.6.0, @emiliom if you agree, can you open up an issue for this?

[emiliom]

Thanks. That makes sense, and I'll open the issue.

[emiliom]

Regarding the time reversal corrections in the combine operation, you and @leewujung have probably already discussed this, but I'm curious if you explicitly came to agreement to also apply time reversal corrections to variables in Vendor_specific? On the one hand, I think of data in that group as being more or less raw and with minimal modifications. On the other hand, since I understand that it's also used in downstream processing (like compute_Sv), I can see that its time coordinates my need to be consistent with those from other groups.

[emiliom]

This looks good. The suggestions I've made don't involve actual problems, except for the simple change to the sonar_model test to handle ES70, ES80 and EA640; they apparently were not being handled previously.

[b-reyes]

Regarding the time reversal corrections in the combine operation, you and @leewujung have probably already discussed this, but I'm curious if you explicitly came to agreement to also apply time reversal corrections to variables in Vendor_specific? On the one hand, I think of data in that group as being more or less raw and with minimal modifications. On the other hand, since I understand that it's also used in downstream processing (like compute_Sv), I can see that its time coordinates my need to be consistent with those from other groups.

We have not discussed this. As it stands right now, this function will apply time reversal to the Vendor_specific group. If this is a cause for concern, I suggest we open an issue and deal with it after 0.6.0.

[b-reyes]

The suggestions I've made don't involve actual problems, except for the simple change to the sonar_model test to handle ES70, ES80 and EA640; they apparently were not being handled previously.

I have committed your change to the sonar_model test to handle ES70, ES80 and EA640. For all of the other comments you have made, I highly suggest that we open up an issue with all of these concerns and deal with them after 0.6.0. As I have stated in the past, this combine_echodata function should be investigated further.

[emiliom]

Thanks. I'll follow up with 1-2 new issues (see my comments). We can merge now, right?

[emiliom]

Almost forgot this:

We have not discussed this. As it stands right now, this function will apply time reversal to the Vendor_specific group. If this is a cause for concern, I suggest we open an issue and deal with it after 0.6.0.

I don't know if it's a cause for concern. I'm fine leaving things as is in this PR, with respect to Vendor_specific

[b-reyes]

Thanks. I'll follow up with 1-2 new issues (see my comments). We can merge now, right?

Yes, I think we can merge now.