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

Update _calc_Sv_offset for AZFP parsing #1304

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Apr 17, 2024

This PR includes the following:

  • adds additional values in the Sv_offset dictionary based on communication with ASL Evn Sci
  • updates _calc_Sv_offset to use a param dictionary instead of nested if-else

One existing test needs to be skipped because the pulse length is not in the Sv_offset dictionary.
We need to request a new file from ASL or Rutgers folks to make sure we still have tests covering files without temperature/pressure/tilt data (those on glider).

This is to address #1300.

@leewujung leewujung added this to the v0.8.4 milestone Apr 17, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.94%. Comparing base (9f56124) to head (b729729).
Report is 44 commits behind head on main.

Files Patch % Lines
echopype/convert/parse_azfp.py 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1304      +/-   ##
==========================================
- Coverage   83.52%   77.94%   -5.58%     
==========================================
  Files          64       16      -48     
  Lines        5686     2621    -3065     
==========================================
- Hits         4749     2043    -2706     
+ Misses        937      578     -359     
Flag Coverage Δ
unittests 77.94% <77.77%> (-5.58%) ⬇️

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.

@leewujung leewujung self-assigned this Apr 17, 2024
Copy link
Collaborator

@ctuguinay ctuguinay left a comment

Choose a reason for hiding this comment

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

I think the changes look good. I am a bit curious about its usage though.

I'm looking at the code found here in parse_groups_azfp.py set_vendor function:

        # Build variables in the output xarray Dataset
        Sv_offset = np.zeros_like(self.freq_sorted)
        for ind, ich in enumerate(self.freq_ind_sorted):
            # TODO: should not access the private function, better to compute Sv_offset in parser
            Sv_offset[ind] = self.parser_obj._calc_Sv_offset(
                self.freq_sorted[ind], unpacked_data["pulse_len"][ich]
            )

Perhaps it can be an independent function, but still in parse_azfp.py, or we can (as the TODO says) create the Sv_offset in the parser and pack it in parser_obj.unpacked_data? I think the latter would be much nicer since set_vendor should just be packing the vendor datasets, and it also gives the function an actual purpose within the parser class, rather than just being in it for encapsulation.

@leewujung
Copy link
Member Author

I think the latter would be much nicer since set_vendor should just be packing the vendor datasets, and it also gives the function an actual purpose within the parser class, rather than just being in it for encapsulation.

Yep, it'll be nice to have that in the parser so that we can keep the set group methods clean. Do you want to make a PR for that change? 😛

@ctuguinay
Copy link
Collaborator

@leewujung Yeah I can do that. Perhaps, I do that PR in conjunction with when the test data arrives, so it's a little more than a few lines changed?

@leewujung
Copy link
Member Author

Sure, let me email ASL and cc you.

@leewujung leewujung merged commit 2c5dd6b into OSOceanAcoustics:main Apr 17, 2024
5 checks passed
@leewujung leewujung deleted the add_Sv_offset branch July 21, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants