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

Move some Vendor-specific group attributes to data variables #1075

Merged
merged 5 commits into from
Jul 14, 2023

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Jul 12, 2023

This PR moves some Vendor-specific group attributes to data variables to make it more efficient to lazy-load and combine echodata objects, and potentially on the writing speed to cloud. This addresses #1055 and potentially #640 (AZFP) and #1045 (though the goal changed, EK80).

The changes include:

  • EK80: move xml_config which is the entire XML string from an attribute to a data variable
  • AZFP: move all Vendor-specific group attributes to data variables
    • some of these variables come from the XML associated with the 01A file
    • some of these variables come from the unpacked ping by ping data
    • a common feature of these variables is that they are not time-varying (which is probably why we saved them as attributes in the first place)

I've tested combine_echodata with the new format and these are combined correctly -- that all these variables remain data variables without a dimension (constants). The combine would error out if any of these variables change across the echodata objects to be combined, which is the desired behavior since those echodata objects should not be combined.

@leewujung leewujung added this to the 0.8.0 milestone Jul 12, 2023
@leewujung leewujung requested a review from emiliom July 12, 2023 16:19
Copy link
Collaborator

@emiliom emiliom 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! Things are so nice and simple when you don't have to worry about having variable attributes 😆 . I like how you organized the variable assignments into blocks with a helpful but brief comment line.

Just one question: I assume that these changes have no downstream effect on calibration functions, since there are no changes to that code?

@leewujung
Copy link
Member Author

Just one question: I assume that these changes have no downstream effect on calibration functions, since there are no changes to that code?

Oh right, this completely slipped my mind. The ones that I can think of on top of my head are the conversion for temperature and tilt_x/tilt_y, from the raw count combining with the coefficients in the variables (_compute_tilt, _compute_temperature). I looked them up and luckily it actually uses the parsed data dict instead of the echodata vendor-specific group entries for that, since the conversion is in the parser. So I guess the changes are not used anywhere. I'll take another look on the ones I moved, just to be sure.

@leewujung
Copy link
Member Author

Ok, confirmed -- none of the attributes that I moved to be data variables are used in later calculations. :)

@leewujung leewujung merged commit e11c665 into OSOceanAcoustics:dev Jul 14, 2023
@leewujung leewujung modified the milestones: 0.8.0, 0.7.2 Jul 15, 2023
lsetiawan pushed a commit to lsetiawan/echopype that referenced this pull request Jul 28, 2023
…Acoustics#1075)

* change EK80 xml in vendor group to be variable instead of attribute

* move azfp vendor attrs to variables

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove noqa

* add back accidentally removed ,

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@leewujung leewujung deleted the vendor-attr-var branch July 21, 2024 00:57
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.

2 participants