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 92.59259% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.40%. Comparing base (9f56124) to head (80b6656).
Report is 152 commits behind head on main.

Files with missing lines	Patch %	Lines
echopype/convert/parse_azfp.py	85.71%	2 Missing ⚠️
echopype/convert/parse_ad2cp.py	80.00%	1 Missing ⚠️
echopype/convert/parse_azfp6.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1399      +/-   ##
==========================================
- Coverage   83.52%   80.40%   -3.12%     
==========================================
  Files          64       18      -46     
  Lines        5686     3138    -2548     
==========================================
- Hits         4749     2523    -2226     
+ Misses        937      615     -322

Flag	Coverage Δ
unittests	`80.40% <92.59%> (-3.12%)`	⬇️

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.

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.

…#1400) Bumps [actions/cache](https://github.com/actions/cache) from 4.1.1 to 4.1.2. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v4.1.1...v4.1.2) --- updated-dependencies: - dependency-name: actions/cache dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…oustics#1401) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5.2.0 to 5.3.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5.2.0...v5.3.0) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

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

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

More merge fixes.

spacetimeengineer · 2024-11-01T14:51:33Z

I wanted to provide some context: I decided to merge a few of my branches since the code was relevant enough. I just added more sonar_model checkers and tests.

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 sonar_model handling, grouping #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

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 and others added 8 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 9 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

spacetimeengineer and others added 4 commits November 1, 2024 10:36

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

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

d128ff7

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

More merge fixes.

48d9d6b

More merge fixes.

spacetimeengineer changed the title ~~Added sonar model checkers for EK60, EK80~~ Added sonar model checkers for EK60, EK80, ER60, AZFP6, AZFP, AD2CP Nov 1, 2024

spacetimeengineer changed the title ~~Added sonar model checkers for EK60, EK80, ER60, AZFP6, AZFP, AD2CP~~ Added sonar model checkers for EK60, EK80, ER60, AZFP6, AZFP, AD2CP and tests Nov 1, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

spacetimeengineer commented Oct 25, 2024

spacetimeengineer left a comment

codecov-commenter commented Oct 25, 2024 •

edited

Loading

spacetimeengineer commented Oct 28, 2024

leewujung commented Oct 30, 2024

spacetimeengineer commented Oct 30, 2024 via email

spacetimeengineer commented Oct 30, 2024 via email

spacetimeengineer commented Oct 30, 2024

leewujung commented Oct 30, 2024

spacetimeengineer commented Oct 30, 2024

spacetimeengineer commented Nov 1, 2024

leewujung commented Nov 1, 2024

spacetimeengineer commented Nov 1, 2024 •

edited

Loading

leewujung Nov 18, 2024

leewujung Nov 18, 2024

leewujung left a comment •

edited

Loading

spacetimeengineer commented Nov 18, 2024

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

Conversation

spacetimeengineer commented Oct 25, 2024

spacetimeengineer left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 25, 2024 • edited Loading

Codecov Report

spacetimeengineer commented Oct 28, 2024

leewujung commented Oct 30, 2024

spacetimeengineer commented Oct 30, 2024 via email

spacetimeengineer commented Oct 30, 2024 via email

spacetimeengineer commented Oct 30, 2024

leewujung commented Oct 30, 2024

spacetimeengineer commented Oct 30, 2024

spacetimeengineer commented Nov 1, 2024

leewujung commented Nov 1, 2024

spacetimeengineer commented Nov 1, 2024 • edited Loading

leewujung Nov 18, 2024

Choose a reason for hiding this comment

leewujung Nov 18, 2024

Choose a reason for hiding this comment

leewujung left a comment • edited Loading

Choose a reason for hiding this comment

spacetimeengineer commented Nov 18, 2024

codecov-commenter commented Oct 25, 2024 •

edited

Loading

spacetimeengineer commented Nov 1, 2024 •

edited

Loading

leewujung left a comment •

edited

Loading