Added sonar model checkers for EK60, EK80, ER60, AZFP6, AZFP, AD2CP and tests #1399

spacetimeengineer · 2024-10-25T19:47:19Z

Future releases of echopype may accommodate cases where the user is not required to provide sonar model numbers. We have here a simple checker to do that.

-Added model checkers for EK60 and EK80 sonar models.

Future releases of echopype may accommodate cases where the user is not required to provide sonar model numbers. We have here a simple checker to do that. -Added model checkers for EK60 and EK80 sonar models.

for more information, see https://pre-commit.ci

spacetimeengineer

It didn't seem appropriate to put this function in the class, at least not yet but the file did seem a good place to put this.

codecov-commenter · 2024-10-25T20:01:47Z

Codecov Report

Attention: Patch coverage is 22.00000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 79.84%. Comparing base (9f56124) to head (c7c4f94).
Report is 211 commits behind head on main.

Files with missing lines	Patch %	Lines
echopype/convert/sonar_model_checker.py	20.40%	39 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1399      +/-   ##
==========================================
- Coverage   83.52%   79.84%   -3.68%     
==========================================
  Files          64       20      -44     
  Lines        5686     3161    -2525     
==========================================
- Hits         4749     2524    -2225     
+ Misses        937      637     -300

Flag	Coverage Δ
unittests	`79.84% <22.00%> (-3.68%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

spacetimeengineer · 2024-10-28T14:09:15Z

I do not have coverage because I haven't wrote the tests yet. Ill be sure to get those over ASAP.

Added test cases for EK60 and EK80 raw parser functions to validate their identification of echosounder types through raw data alone.

The test failed because a proper path wasn't provided to them. This commit should fix that.

I forgot to import the outside-of-class functions in the __init__. Consider this fixed.

- Added notes about the potential need to remove sudo privileges for some users. - Improved tests by adding an exception handler for key errors, returning False when encountered.

Syntax error fixed.

The 'e' variable commonly used in error handling was not utilized, so I removed it. This change caused tests to fail, prompting its removal.

leewujung · 2024-10-30T20:50:38Z

@spacetimeengineer: is this PR ready to review? If not just ping me when you think it's ready. Thanks!

spacetimeengineer · 2024-10-30T20:59:48Z

It is! I am still navigating the CI logs and I am not sure what is failing.

…

On Wed, Oct 30, 2024 at 4:51 PM Wu-Jung Lee ***@***.***> wrote: @spacetimeengineer <https://github.com/spacetimeengineer>: is this PR ready to review? If not just ping me when you think it's ready. Thanks! — Reply to this email directly, view it on GitHub <#1399 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA26MX7EEDFZSZRK2WBC573Z6FBDJAVCNFSM6AAAAABQT5TI66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGM2TCNRQGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Michael Ryan, *Engineer, Physicist(508) 617-3738 * *linkedin.com/in/mikeryanengineer <http://linkedin.com/in/mikeryanengineer>* *github.com/spacetimeengineer <http://github.com/spacetimeengineer>* *stackexchange.com/users/4492334/spacetimeengineer <http://stackexchange.com/users/4492334/spacetimeengineer>*

spacetimeengineer · 2024-10-30T21:01:10Z

I should have said that it passes the local test but the post commit tests hosted by github are failing. On Wed, Oct 30, 2024 at 4:59 PM Spacetime Engineer < ***@***.***> wrote:

…

It is! I am still navigating the CI logs and I am not sure what is failing. On Wed, Oct 30, 2024 at 4:51 PM Wu-Jung Lee ***@***.***> wrote: > @spacetimeengineer <https://github.com/spacetimeengineer>: is this PR > ready to review? If not just ping me when you think it's ready. Thanks! > > — > Reply to this email directly, view it on GitHub > <#1399 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AA26MX7EEDFZSZRK2WBC573Z6FBDJAVCNFSM6AAAAABQT5TI66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGM2TCNRQGY> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- Michael Ryan, *Engineer, Physicist(508) 617-3738 * *linkedin.com/in/mikeryanengineer <http://linkedin.com/in/mikeryanengineer>* *github.com/spacetimeengineer <http://github.com/spacetimeengineer>* *stackexchange.com/users/4492334/spacetimeengineer <http://stackexchange.com/users/4492334/spacetimeengineer>*

-- Michael Ryan, *Engineer, Physicist(508) 617-3738 * *linkedin.com/in/mikeryanengineer <http://linkedin.com/in/mikeryanengineer>* *github.com/spacetimeengineer <http://github.com/spacetimeengineer>* *stackexchange.com/users/4492334/spacetimeengineer <http://stackexchange.com/users/4492334/spacetimeengineer>*

spacetimeengineer · 2024-10-30T21:04:25Z

On my fork I have having these errors pop up and I don't fully understand them yet.

minioci-build
Username and password required
http-build
The job was canceled because "minioci" failed.
http-build
The operation was canceled.

leewujung · 2024-10-30T21:22:16Z

Oh I see - you can disable these builds since they will be run on the main repo (here).

spacetimeengineer · 2024-10-30T22:57:18Z

Thanks, that’s great! Let me know if there's anything else I need to do. Just a heads-up—this PR is part of a series of similar checkers for other models. I don’t think this approach will be very effective until all models are covered, and even then, I’d recommend keeping the sonar_model parameter as an option at least. Let me know if this seems a worthwhile feature.

… file directory. For EK60 only.

Added better tests for ek80 checker functions.

Added the rest of the sonar checkers.

Added more tests for azfp and ad2cp.

Updated tests.

…remaining-sonar-models-from-raws

for more information, see https://pre-commit.ci

Sorry about that.

…github.com/spacetimeengineer/echopype into Parse-sonar-model-from-.raw-for-EK60,-EK80

leewujung · 2024-11-01T14:57:32Z

I haven't had a chance to check the code, but from a quick glance this is fine since it is not a gigantic PR.

I think it would be better to always keep the sonar_model argument, and spit out an error if what they specified mismatches with what are contained in the files (i.e. I don't think we should "make it work" under the hood for users if they specify the wrong parameters). It is part of making sure people are aware of what operations they are trying to perform, since some people may be new to echosounder processing.

spacetimeengineer · 2024-11-01T15:12:03Z

I agree with your perspective. My intention was not to remove the sonar model parameter, (it only helps echopype users to provide more information) but rather to explore the possibility of allowing future releases to parse the sonar model directly from the raw data. This approach could enhance the intelligence of the base parser. While it wouldn't be fully effective until all model checkers are developed, it would allow us to clarify what the sonar model is not, rather than just asserting what it is. I figured if the user makes a mistake providing the sonar_model then the more intelligent base parser could provide a warning and ask the user if they want to process with the true parsed sonar_model value or some kind of override.

Need to import these functions.

leewujung · 2024-11-18T14:32:58Z

docs/source/contributing.md

-Docker image on Docker hub.
+Docker image on Docker hub. There’s no need to pull directly from Docker Hub, as the scripts below will handle that for you. For Linux Users: If you run into an issue where accessing the Docker daemon requires sudo privileges, which may not suit your environment, you can adjust your settings to allow non-root access to Docker:
+
+```shell
+sudo groupadd docker
+```
+
+Add the current user to the Docker group
+```shell
+sudo gpasswd -a $USER docker
+```
+
+Restart the docker daemon
+```shell
+sudo service docker restart
+```
+
+You might also need to restart your computer if the steps above don't resolve the issue.


Could you move this to a new PR since it's under a completely different topic? I think there we could update the contributing guide from you and others as you're fresh from setting up a dev environment and may want to add other details. Thanks!

leewujung · 2024-11-18T15:11:14Z

echopype/convert/parse_ek60.py

+    with RawSimradFile(raw_file, "r", storage_options=storage_options) as fid:
+        config_datagram = fid.read(1)
+        config_datagram["timestamp"] = np.datetime64(
+            config_datagram["timestamp"].replace(tzinfo=None), "[ns]"
+        )


I am not sure why you are accessing the timestamp here.

See Gavin's #494 (comment) that I think would be useful here to check the format without using the full blown parser. Implementing that into the checker would address #494 too in this PR.

leewujung

@spacetimeengineer : Thanks for the PR! I added some inline comments above, and have a few structural comments below:

I think the sonar model checking code should be in a separate module (say convert/sonar_model_checker.py), specified in core.py like the parser and set_group functions, and called in open_raw to check the matches between user-specified sonar_model type and the actual file content. The checks can happen right after the _check_file call that checks file existence and before the parser is called to parse the full data file. This way the functions will be easier to maintain since they don't get wrapped in the parser child classes but with different method names.

Similarly, I think the tests for these checker functions can be in a different module tests/convert/test_sonar_model_checker.py, to keep these tests isolated from the actual parsing of data file content.

The is_{SONAR_MODEL} functions you added are of 2 types:

One checks for just the file extension name: is_AD2CP, is_AZFP6 -- this check already exists in functions validate_ext and validate_azfp_ext in core.py.
One takes a peek "under the hood" to see if the data actually fits the expected config format: is_AZFP, is_EK60, is_ER60, is_EK80 -- we need these. Note that the older Simrad format was also used for many other echosounder models, not just EK60 and ER60. The same for the newer Simrad format. We can improve what's in core.py so that the same blocks using the same parser/setgrouper don't need to be repeated. ~~I'll write a small issue for that -- we just need a mechanism to select the right key in the main SONAR_MODELS dict.~~ -- in fact, this is just #995.

For paths to test files, please use fixtures:

echopype/echopype/tests/conftest.py

Lines 15 to 16 in 2813a25

    
           @pytest.fixture(scope="session") 
        
           def test_path():

This will make maintenance and upcoming migration of the testing infrastructure smoother.

spacetimeengineer · 2024-11-18T16:21:42Z

@leewujung Acknowledged. Ill begin putting in these changes.

Finishing 1399 requests from Dr. Wu-Jung

for more information, see https://pre-commit.ci

Adding more PR requests.

Adding in more requests.

for more information, see https://pre-commit.ci

PR1399

Fixed test_convert_ek80.py

Made more updates to tests.

for more information, see https://pre-commit.ci

…rom-.raw-for-EK60,-EK80

spacetimeengineer · 2025-03-18T13:46:19Z

@leewujung My team has asked me to get this PR merged because the sonar-checking code is essential for something they’re working on. I suggested moving the code to their library, but they couldn’t do so due to large dependencies. I understand their perspective, but I don’t want to impose this on you or anyone. If there’s anything I can do to improve this PR, please let me know.

leewujung · 2025-03-18T14:05:27Z

@spacetimeengineer : I was never sure if the requested changes were done since my comment back in Nov, and if not, your reasoning. I quickly checked it and think many of my requests were not included.

Pinging @jmjech since I can't merge code without discussions.

Note I am not able to work on this until next week due to other responsibilities.

spacetimeengineer · 2025-03-18T14:28:53Z

@leewujung I think there may have been some miscommunication, and I take full responsibility for that. Here’s my understanding: Originally, this PR aimed to enhance the functionality of the echodata object, but you pointed out that it wasn’t necessary (which I agreed with after your explanation). However, there were still potential uses for the code, either for future features or as utility functions.

At that point, I understood that the PR’s purpose had shifted, so I may not have fully met some of your initial requirements since I was no longer modifying the echodata object’s algorithm but perhaps you were trying to help me get these sonar checking functions in the main code. (Of course, if you’d like me to do still that, I can.) Later, the intentions of this PR changed again when I realized someone else needed these functions but found the dependencies too large. Given that, it didn’t seem entirely unreasonable to resubmit.

This is a small PR—it only includes the sonar-checking functions in the specified files, along with the corresponding tests which I believe work. Thats all it was meant to be. So the only requirements from you I could meet was the location of the code and the files names you specified. If I missed anything, let me know, and I’ll fix it.

spacetimeengineer and others added 3 commits October 25, 2024 15:33

Added sonar model checkers for EK60, EK80

94ec110

Future releases of echopype may accommodate cases where the user is not required to provide sonar model numbers. We have here a simple checker to do that. -Added model checkers for EK60 and EK80 sonar models.

[pre-commit.ci] auto fixes from pre-commit.com hooks

cae865e

for more information, see https://pre-commit.ci

Update parse_ek60.py

2cb2625

spacetimeengineer commented Oct 25, 2024

View reviewed changes

spacetimeengineer added 6 commits October 28, 2024 10:19

Added test cases for EK60 and EK80 model validation

7bf89a1

Added test cases for EK60 and EK80 raw parser functions to validate their identification of echosounder types through raw data alone.

Fixed the tests.

1affd2d

The test failed because a proper path wasn't provided to them. This commit should fix that.

Fixed an import error

2abe8be

I forgot to import the outside-of-class functions in the __init__. Consider this fixed.

Test fix + Update to documentation for Docker and sudo privileges

fa7dc1c

- Added notes about the potential need to remove sudo privileges for some users. - Improved tests by adding an exception handler for key errors, returning False when encountered.

Update parse_ek60.py

adb5111

Syntax error fixed.

Refactor error handling: Removed unnecessary 'e' variable

8967c3e

The 'e' variable commonly used in error handling was not utilized, so I removed it. This change caused tests to fail, prompting its removal.

leewujung self-requested a review October 30, 2024 20:50

spacetimeengineer and others added 11 commits October 31, 2024 09:36

Made tests better so that they run through all test files in the test…

907424d

… file directory. For EK60 only.

Added better tests for ek80 checker functions.

8a2517d

Added better tests for ek80 checker functions.

Added the rest of the sonar checkers.

01ceb52

Added the rest of the sonar checkers.

Update test_convert_azfp6.py

0534589

Added more tests

853d01b

Added more tests for azfp and ad2cp.

Update test_convert_azfp.py

d3467f5

Corrected some tests.

bf73faa

Updated tests.

Merge branch 'Parse-sonar-model-from-.raw-for-EK60,-EK80' into parse-…

a8cd12b

…remaining-sonar-models-from-raws

[pre-commit.ci] auto fixes from pre-commit.com hooks

4db7dd5

for more information, see https://pre-commit.ci

Needed some fixes for the merge.

9929523

Sorry about that.

Merge branch 'Parse-sonar-model-from-.raw-for-EK60,-EK80' of https://…

1f8f44d

…github.com/spacetimeengineer/echopype into Parse-sonar-model-from-.raw-for-EK60,-EK80

spacetimeengineer self-assigned this Nov 1, 2024

spacetimeengineer added the enhancement This makes echopype better label Nov 1, 2024

Update __init__.py

80b6656

Need to import these functions.

leewujung mentioned this pull request Nov 9, 2024

sonar_model handling, grouping #995

Open

leewujung reviewed Nov 18, 2024

View reviewed changes

leewujung requested changes Nov 18, 2024

View reviewed changes

spacetimeengineer and others added 6 commits December 18, 2024 12:11

Finishing 1399 requests from Dr. Wu-Jung

29ce582

Finishing 1399 requests from Dr. Wu-Jung

[pre-commit.ci] auto fixes from pre-commit.com hooks

20d0932

for more information, see https://pre-commit.ci

Adding more PR requests.

dea1f3c

Adding more PR requests.

Adding in more requests.

35f6e0f

Adding in more requests.

[pre-commit.ci] auto fixes from pre-commit.com hooks

afd3bd2

for more information, see https://pre-commit.ci

More fixes for PR1399

4574f81

PR1399

leewujung marked this pull request as draft December 30, 2024 19:40

spacetimeengineer and others added 9 commits January 13, 2025 11:56

Update sonar_model_checker.py

895b764

Update __init__.py

1049942

Fixed test_convert_ek80.py

e39263b

Fixed test_convert_ek80.py

Made more updates to tests.

cdcb936

Made more updates to tests.

Merge branch 'main' into Parse-sonar-model-from-.raw-for-EK60,-EK80

8d9562d

[pre-commit.ci] auto fixes from pre-commit.com hooks

3a89a9c

for more information, see https://pre-commit.ci

Update test_convert_ad2cp.py

5dfd4d4

Update test_sonar_model_checker.py

0d63fd3

Merge remote-tracking branch 'upstream/main' into Parse-sonar-model-f…

c7c4f94

…rom-.raw-for-EK60,-EK80

spacetimeengineer marked this pull request as ready for review March 17, 2025 17:23

Added sonar model checkers for EK60, EK80, ER60, AZFP6, AZFP, AD2CP and tests #1399

Are you sure you want to change the base?

Added sonar model checkers for EK60, EK80, ER60, AZFP6, AZFP, AD2CP and tests #1399

Uh oh!

Conversation

spacetimeengineer commented Oct 25, 2024

Uh oh!

spacetimeengineer left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

spacetimeengineer commented Oct 28, 2024

Uh oh!

leewujung commented Oct 30, 2024

Uh oh!

spacetimeengineer commented Oct 30, 2024 via email

Uh oh!

spacetimeengineer commented Oct 30, 2024 via email

Uh oh!

spacetimeengineer commented Oct 30, 2024

Uh oh!

leewujung commented Oct 30, 2024

Uh oh!

spacetimeengineer commented Oct 30, 2024

Uh oh!

leewujung commented Nov 1, 2024

Uh oh!

spacetimeengineer commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leewujung Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

leewujung Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

leewujung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacetimeengineer commented Nov 18, 2024

Uh oh!

spacetimeengineer commented Mar 18, 2025

Uh oh!

leewujung commented Mar 18, 2025

Uh oh!

spacetimeengineer commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Oct 25, 2024 •

edited

Loading

spacetimeengineer commented Nov 1, 2024 •

edited

Loading

leewujung left a comment •

edited

Loading