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

AZFP: add missing mandatory Beam_group1 variables #1102

Merged
merged 4 commits into from
Aug 4, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Jul 30, 2023

Addresses #1100

  • beam_type
  • transmit_type
  • beam_direction_x/y/z

@emiliom emiliom marked this pull request as draft July 30, 2023 19:46
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2023

Codecov Report

Merging #1102 (ec6e95e) into dev (4250acf) will decrease coverage by 0.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1102      +/-   ##
==========================================
- Coverage   78.15%   77.65%   -0.50%     
==========================================
  Files          65       18      -47     
  Lines        6234     2927    -3307     
==========================================
- Hits         4872     2273    -2599     
+ Misses       1362      654     -708     
Flag Coverage Δ
unittests 77.65% <ø> (-0.50%) ⬇️

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

Files Changed Coverage Δ
echopype/convert/set_groups_azfp.py 97.61% <ø> (ø)

... and 49 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@emiliom emiliom self-assigned this Jul 31, 2023
@emiliom emiliom added the data format Anything about data format label Jul 31, 2023
@emiliom emiliom added this to the 0.8.0 milestone Jul 31, 2023
@emiliom
Copy link
Collaborator Author

emiliom commented Jul 31, 2023

Note to self: When addressing beam_direction_x/y/z, it may be helpful to refer back to the AZFP Platform group overhaul PR's merged in the last couple of months, #1058 and #1061

@emiliom
Copy link
Collaborator Author

emiliom commented Aug 4, 2023

Added beam_direction_x/y/z variables, set to nan. That is, nothing is being read from AZFP data. The variables are created because they're mandatory (M) in the convention.

@emiliom emiliom marked this pull request as ready for review August 4, 2023 02:14
@emiliom emiliom changed the title AZFP: add missing mandatory Beam_group1 attributes AZFP: add missing mandatory Beam_group1 variables Aug 4, 2023
"long_name": "Type of transmitted pulse",
"flag_values": ["CW"],
"flag_meanings": [
"Continuous Wave",
Copy link
Member

Choose a reason for hiding this comment

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

I've been mulling over about this. It is true that CW is Continuous Wave, but it is an approximation. The actual transmit signal is gated sine wave. I wonder if this would be misleading. This is something defined in the convention, but do you think we can add something to make it "Continuous Wave (gated sine wave)"?

Copy link
Member

@leewujung leewujung 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! I only have a small comment on one of the flag_meanings.

@emiliom
Copy link
Collaborator Author

emiliom commented Aug 4, 2023

We've discussed the flag_meanings comment. We decided to stick with "CW", but a new PR will have a more fleshed out flag_meanings entry that better addresses the concern.

@emiliom emiliom merged commit 389802a into OSOceanAcoustics:dev Aug 4, 2023
@emiliom emiliom deleted the azfp-missing-beamvars branch August 4, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data format Anything about data format
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants