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

test echo range for EK80 reduced sample rate #1270

Closed
wants to merge 1 commit into from

Conversation

MohamedNasser8
Copy link

@MohamedNasser8 MohamedNasser8 commented Feb 29, 2024

Fixes #969
I don't know if the mean by reduced sampling rate is the reduced file size, if it is, The fifth test case do the same thing as me.
Or I should resample the data?

@leewujung
Copy link
Member

@MohamedNasser8 : My bad, I did not put in enough info the original description in #969.

Here the reduced sampling rate refers to a specific setup the EK80 instrument allows. That setup would make the discretization along the range_sample axis coarser in the EchoData object (from the open_raw function), which affects the echo_range values in the output of compute_Sv. The intention of #969 is to check the calculation against the expected values from another program. I got some sample data from a colleague from #702, and will put that somewhere for you to download, and the expected values it should be tested against.

@MohamedNasser8
Copy link
Author

@leewujung Thanks for your illustration, I'm waiting for the data to test it.

@leewujung leewujung added this to the v0.8.4 milestone Mar 14, 2024
@MohamedNasser8
Copy link
Author

Hello @leewujung, I just wanted to remind you about the data I can test with.

@leewujung leewujung self-assigned this Apr 18, 2024
@leewujung leewujung modified the milestones: v0.8.4, v0.9.0 Apr 22, 2024
@ctuguinay ctuguinay modified the milestones: v0.9.0, v0.9.1 Apr 30, 2024
@ctuguinay
Copy link
Collaborator

I'm going to close this. I think this should probably be tested when we get in-lab EK80 systems in conjunction with the many other possible configurations @leewujung

@ctuguinay ctuguinay closed this Aug 2, 2024
@leewujung
Copy link
Member

yeah but probably should keep this open as a reminder?

@ctuguinay
Copy link
Collaborator

Ah yeah I see

@ctuguinay ctuguinay reopened this Aug 2, 2024
@leewujung
Copy link
Member

Looking at this again I agree with you @ctuguinay that we should just close this. We have #969 as a reminder.

@leewujung leewujung closed this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants