Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/add sv #38

Merged
merged 6 commits into from
Sep 18, 2023
Merged

Feature/add sv #38

merged 6 commits into from
Sep 18, 2023

Conversation

simedroniraluca
Copy link
Collaborator

No description provided.

@mihaiboldeanu mihaiboldeanu requested review from beatfactor and mihaiboldeanu and removed request for mihaiboldeanu and beatfactor September 14, 2023 12:58
Copy link
Collaborator

@ruxandra-valcu ruxandra-valcu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like the Sv file to also include metadata: lat/lon, depth and beam angle data, for later use. All these can be found in the echopype.consolidate module

@simedroniraluca
Copy link
Collaborator Author

simedroniraluca commented Sep 14, 2023

I would like the Sv file to also include metadata: lat/lon, depth and beam angle data, for later use. All these can be found in the echopype.consolidate module

@ruxandra-valcu Given that the add_depth(), add_location(), and add_splitbeam_angle() functions from the echopype.consolidate module have default parameters that work independently of the optional parameters in Sv, I'm thinking it might not be the best practice to include depth, location, and splitbeam angle right here. What if we create a separate L2 module for that? This would give us an Sv with more flexibility. Also, I feel loading up the compute_sv() function with extra logic beyond its main task might not be the way to go. What do you think?

@ruxandra-valcu
Copy link
Collaborator

ruxandra-valcu commented Sep 15, 2023

This would give us an Sv with more flexibility. Also, I feel loading up the compute_sv() function with extra logic beyond its main task might not be the way to go. What do you think?

Sure, sounds good. The splitbeam in particular is stuff we'll need for false bottom detection so it was more on my mind, but you're right that adding all the functionality would be better. I'll make an issue

@ruxandra-valcu ruxandra-valcu self-requested a review September 15, 2023 04:43
Copy link
Collaborator

@ruxandra-valcu ruxandra-valcu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good

@simedroniraluca
Copy link
Collaborator Author

This would give us an Sv with more flexibility. Also, I feel loading up the compute_sv() function with extra logic beyond its main task might not be the way to go. What do you think?

Sure, sounds good. The splitbeam in particular is stuff we'll need for false bottom detection so it was more on my mind, but you're right that adding all the functionality would be better. I'll make an issue

I will take the issue to resolve (maybe I will need this PR merged for a better integration)

Copy link
Collaborator

@mihaiboldeanu mihaiboldeanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good it just has the same limitation with this comment .

The test should be split into fixtures for getting the files. Maybe that could be added to conftest.py to have all file download/manipulations in a common place as they may be used in multiple locations.

@simedroniraluca
Copy link
Collaborator Author

The code looks good it just has the same limitation with this comment .

The test should be split into fixtures for getting the files. Maybe that could be added to conftest.py to have all file download/manipulations in a common place as they may be used in multiple locations.

About the sonar models part: In echopype, only some models can compute Sv. Though many provide echodata, not all support Sv computation.

@ruxandra-valcu
Copy link
Collaborator

The test should be split into fixtures for getting the files. Maybe that could be added to conftest.py to have all file download/manipulations in a common place as they may be used in multiple locations.

Opened a ticket for that at #40

Copy link
Collaborator

@mihaiboldeanu mihaiboldeanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@mihaiboldeanu mihaiboldeanu merged commit 7859d0c into dev Sep 18, 2023
1 check passed
@beatfactor beatfactor deleted the feature/add-sv branch September 25, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants