Added alpha-version code for new AZFP6 format #1323
Conversation
|
Hey @dash-uvic : Thanks for the contribution! Could you fix the pre-commit errors? Also let us know what files you would like to add to the CI. As always, small files are better! |
|
Fixed pre-commit errors. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
===========================================
- Coverage 83.52% 48.48% -35.04%
===========================================
Files 64 69 +5
Lines 5686 6472 +786
===========================================
- Hits 4749 3138 -1611
- Misses 937 3334 +2397
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
echopype/convert/set_groups_azfp6.py
Outdated
| from .set_groups_base import SetGroupsBase | ||
|
|
||
|
|
||
| class SetGroupsAZFP6(SetGroupsBase): |
There was a problem hiding this comment.
@dash-uvic : It seems that a lot of the code here are copy-pasted here from SetGroupsAZFP? If that's the case, why don't you inherit from that and can only change the parts that are different? (Note I have not gone in detail to compare what was changed).
There was a problem hiding this comment.
Since the hardware is still in flux (so Sv values, battery calculations, etc. may change) I decided to keep it separate for now, and then create a base class both ULS5/6 (both in groups and converter classes) would inherit from if we end up going with the current setup. You're right, the AZFP6 group has not changed a lot, and I don't think it'll change much in the future so I rewrite that to inherit from the original AZFP group
There was a problem hiding this comment.
To make sure I understand, the approach you are taking is to have 2 completely separate class for now and then will merge the common components to have a base class for both after the hardware is settled?
There was a problem hiding this comment.
Yes, that's my intention. Or if not a base class, inherit from the current AZFP class but as the class structure will remain the same, even if the internal function changes, so it makes sense to abstract and inherit. If there is a preference for how you'd like the code structure, I'm flexible.
There was a problem hiding this comment.
I think both would be fine. It seems that inheriting from a common based class into separate ULS5/6 classes would make it pretty extensible (not that anyone wants to think about more changes at the moment... lol), so we can go with that. :)
|
Hey @dash-uvic : Thanks for fixing the pre-commits. I've just approved for the CI to run on this PR. If you could add file conversion tests to run on some example test files, that will allow us to review the changes more easily. The way we organized test data now is for you to share the files with us with the specific path you would like those to be in under |
for more information, see https://pre-commit.ci
|
@dash-uvic : I saw that you added some tests and just "approved" the workflow to run - I think usually for a PR it just needs to be approved once and then it'll always run for new changes. If you see that stalled somehow feel free to ping me directly. |
|
@leewujung What's the best method for adding/sending you the test files (average ~50M but there is a larger ~400M file)? |
|
@dash-uvic : Is there a reason why the files need to be that large? If not, could you generated some smaller files? Large files will make CI runs a lot slower. |
|
@leewujung Yeah I can probably truncate the larger azfp file down to fewer timesteps and remove unrequired fields from the expected output Mat file. I think doing that I can get them down to ~10M if that's sufficient. |
|
@dash-uvic : Yes ~10MB would be nice. Thanks! Also if it is easy for you to put the files on a cloud folder somewhere, I can download them and put them in the docker image we currently use for the CI. We would like to move all files to use the github release assets, but don't have time to do it just yet... |
|
@leewujung Ok sounds goods, I'll put them on a cloud and email you the link next week. |
|
@dash-uvic : Thanks for sending the test files. I added them to the CI. Looks like the tests all pass now! Could you please fix that Should we merge this afterwards and call it alpha, or do you have additional changes you would like to add? |
|
@leewujung The pre-commit error has been fixed. Strangely, it did not pop up when I ran pre-commit locally. You can merge this as alpha. I will likely have changes in the future but that won't be for a while and can be a new PR. |
|
Great, thanks @dash-uvic! I wonder if that may be because of different versions of the pre-commit hooks. I'll merge this now and add azfp6 alpha in the documentation. We will release v0.9.0 soon and this will be included in there. :) |
Added in initial code for the new AZFP6 sensor format for *.azfp files from ASL Environmental services. The data is in a new format and has additional optional GPS information. The XML file is now embedded into the data file. No tests added.