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

Incorrect check for waveform_mode and encode_mode parameters in compute_sv() #45

Closed
simedroniraluca opened this issue Sep 18, 2023 · 5 comments
Assignees
Labels
bug Something isn't working L2

Comments

@simedroniraluca
Copy link
Collaborator

The current implementation of compute_sv() requires that the optional parameters waveform_mode and encode_mode appear exclusively for EK80. However, this behavior is not entirely correct.

Expected Behavior:

  1. For EK80: Both waveform_mode and encode_mode are required.
  2. For EK60: While these parameters can be present, they have a fixed configuration of waveform_mode="CW" and encode_mode="power".

Current Behavior:
The function raises an error if waveform_mode and encode_mode are present for EK60, even if they have the correct configuration.

Suggested Fix:
Update the checks in compute_sv() to account for the possibility of these parameters being present in the EK60 case with the previously mentioned configuration.

@simedroniraluca simedroniraluca self-assigned this Sep 18, 2023
@simedroniraluca simedroniraluca added the bug Something isn't working label Sep 18, 2023
@ruxandra-valcu ruxandra-valcu added this to the OceanStream v0.2 milestone Sep 19, 2023
@simedroniraluca
Copy link
Collaborator Author

We only have clear info about the EK80 for waveform_mode and encode_mode. So, I'm only setting rules for EK80. Check the documentation here. We're not totally sure if all EK60 types use the same settings for these parameters.

@ruxandra-valcu
Copy link
Collaborator

Because all EK60 echosounders are narrowband devices, they will all use "CW" and "power"

@simedroniraluca
Copy link
Collaborator Author

Because of the phrases emphasized in the image below, taken from echopype documentation for computing Sv, I do not add checks on other instruments besides EK80. We do not know everything about all types of EK60. We will use our assumption that "For EK60: While these parameters can be present, they have a fixed configuration of waveform_mode='CW' and encode_mode='power', but not in checks.

Image

@simedroniraluca
Copy link
Collaborator Author

#46

@ruxandra-valcu
Copy link
Collaborator

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working L2
Projects
None yet
Development

No branches or pull requests

2 participants