Generalize echopype software provenance and apply to Sv and MVBS #621

emiliom · 2022-04-10T21:49:20Z

Addresses #618. Let's keep it as a draft PR for now, for discussion.

I've abstracted the assignment of the current Provenance software attributes into a function in the new module utils.prov.py. This function now handles both conversion_* and processing_* attributes in a single place. Also eliminated existing duplication of attribute assignments.

Note the additional (not from the convention) global attribute I've added as a suggestion for Sv and MVBS: processing_function. It would allow us to record the actual function used.

Doesn't address the move from src_filenames global attribute to a source_filenames variable (#620)

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

codecov-commenter · 2022-04-10T22:06:39Z

Codecov Report

Merging #621 (c1ebfb3) into dev (6bec3e4) will decrease coverage by 9.17%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##              dev     #621      +/-   ##
==========================================
- Coverage   78.75%   69.57%   -9.18%     
==========================================
  Files          42       37       -5     
  Lines        3784     3744      -40     
==========================================
- Hits         2980     2605     -375     
- Misses        804     1139     +335

Flag	Coverage Δ
unittests	`69.57% <65.21%> (-9.18%)`	⬇️

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

Impacted Files	Coverage Δ
echopype/echodata/combine.py	`63.55% <0.00%> (-10.40%)`	⬇️
echopype/utils/prov.py	`0.00% <0.00%> (ø)`
echopype/calibrate/api.py	`60.41% <63.63%> (-23.37%)`	⬇️
echopype/preprocess/api.py	`74.19% <92.30%> (-16.01%)`	⬇️
echopype/convert/set_groups_base.py	`88.04% <100.00%> (-2.28%)`	⬇️
echopype/convert/set_groups_ek60.py	`91.58% <100.00%> (ø)`
echopype/preprocess/noise_est.py	`73.68% <100.00%> (-20.61%)`	⬇️
echopype/utils/coding.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/echodata/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/calibrate/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 25 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

leewujung · 2022-04-12T16:23:08Z

echopype/utils/prov.py

+    }
+    if source_files:
+        # TODO: src_filenames will be replaced with a new variable, source_filenames
+        #   Also, come to think of it, source files is not "echopype provenance" info per se


Do you mean that source_files is part of the convention and not from echopype? Just wanted to make sure I understand this.

What I meant with "source files is not "echopype provenance" info per se" was that all the other attributes I've included in echopype_prov_attrs are about the echopype software itself, but source files is not. Though, come to think of it, strictly speaking, conversion_time is not either .... Hmm. We could rename echopype_prov_attrs to something more general. Its scope is the software used (name & version) and basic info about its application (timestamp when it was run, and input files used -- though the latter will be moved to a variable and will no longer be an attribute)

Looking at this again and seeing how processing_function is currently separately done in calibrate.api._compute_cal, I wonder if we could rename this to echopype_ver_time_files_prov so that it is clear what this function does, for when we come back to provenance in the future.

Sure, sounds good. It sounds like you're also saying that we keep the source_files provenance functionality in this function, even though it'll be changed to add xarray variables rather than an attribute. Correct? I haven't thought through the consequences of doing that for this function, given that echopype_prov_attrs currently just populates a dict and returns it.

It sounds like you're also saying that we keep the source_files provenance functionality in this function, even though it'll be changed to add xarray variables rather than an attribute. Correct?

I didn't think that far... ha. I think you are right that since it will be a data variable not an attribute, it should not be here. In that case, the function name would become echopype_ver_time_prov.

emiliom · 2022-04-21T16:31:58Z

Note the additional (not from the convention) global attribute I've added as a suggestion for Sv and MVBS: processing_function. It would allow us to record the actual function used.

@leewujung this is what I was referring to.

emiliom · 2022-04-21T17:46:33Z

TODO: Add software provenance attributes to remove_noise output dataset. I'll add a commit to this PR.

emiliom · 2022-04-28T16:35:12Z

TODO: For EK80, add waveform_mode and encode_mode parameters passed to compute_Sv and compute_TS (and pass them to MVBS too?). These parameters are not used in any other sensors at this time, so no attributes will be added.

leewujung · 2022-04-28T18:30:03Z

Yes please pass these to MVBS also, as part of the chain of provenance (is there a specific term for this chain?).

… to MVBS

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

emiliom · 2022-05-01T21:52:49Z

@leewujung I've pushed several commits that include multiple improvements, some of which are not about provenance per se.

Removed the sv variable being added in compute_MVBS_index_binning by mistake
Add provenance global attributes for remove_noise
For EK80, add to Sv the waveform_mode and encode_mode parameters passed to compute_Sv and compute_TS
Add variable attributes to Sv_corrected and Sv_noise (from remove_noise), and add actual_range attribute to Sv_corrected, Sv_noise and MVBS Sv. This is more related to Add variable Sv attributes to output dataset from compute_Sv #582 than to Add echopype software and version provenance attributes to Sv and MVBS datasets #618

I decided not to try to implement "provenance chain", where, say, parameters used in computing Sv are passed downstream to MVBS. I think that's a larger topic. Let's revisit it after 0.6.0 is out, at some point when we focus on provenance again.

echopype/convert/set_groups_ek60.py

echopype/preprocess/noise_est.py

echopype/preprocess/api.py

leewujung

I pushed a change to remove the unused datetime import since GH doesn't allow me to comment on unchanged code.

I added a few suggestions, but otherwise this looks ready to go!

FYI- I pondered a bit about the long_name you added for Sv_noise and Sv_corrected, but decided that what you have is appropriate.

emiliom · 2022-05-05T21:22:18Z

I pushed a change to remove the unused datetime import since GH doesn't allow me to comment on unchanged code.

Thanks!

FYI- I pondered a bit about the long_name you added for Sv_noise and Sv_corrected, but decided that what you have is appropriate.

Great. And yeah, I struggled a bit with that.

I added a few suggestions, but otherwise this looks ready to go!

Great! But since I've made some follow-up comments to your inline comments, I'll give you more time to react. Let's plan to merge today, if possible.

leewujung · 2022-05-05T21:25:11Z

I think I reacted to everything, haha. I can't wait to see this merged!! 😎

emiliom · 2022-05-05T21:32:27Z

Thank you!! Merging now

emiliom added 2 commits April 10, 2022 13:07

Generalize creation of echopype software provenance in new prov module

51bd6d9

Apply echopype provenance to Sv and MVBS

7b6bf92

emiliom added the data format Anything about data format label Apr 10, 2022

emiliom added this to the 0.6.0 milestone Apr 10, 2022

emiliom requested a review from leewujung April 10, 2022 21:49

emiliom self-assigned this Apr 10, 2022

emiliom marked this pull request as draft April 10, 2022 21:49

pre-commit-ci bot and others added 3 commits April 10, 2022 21:49

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

68015e4

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

Remove unused imports in combine

388fa0a

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

22524b2

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

leewujung reviewed Apr 12, 2022

View reviewed changes

emiliom and others added 6 commits April 29, 2022 18:26

Don't add sv to ds_Sv in compute_MVBS_index_binning

af5ea08

Add provenance attributes to remove_noise

4925642

Merge from dev, resolving conflicts

980ed28

Add attributes, provenance to Sv_corrected and Sv_noise, actual_range…

b84d84f

… to MVBS

Add waveform_mode and encode_mode provenance to compute_Sv/TS

bb5530a

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

c46d118

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

emiliom marked this pull request as ready for review May 2, 2022 17:39

remove unused import that caused flake8 check failure

c1ebfb3

leewujung reviewed May 5, 2022

View reviewed changes

echopype/convert/set_groups_ek60.py Show resolved Hide resolved

leewujung reviewed May 5, 2022

View reviewed changes

echopype/preprocess/noise_est.py Show resolved Hide resolved

leewujung reviewed May 5, 2022

View reviewed changes

echopype/preprocess/api.py Show resolved Hide resolved

leewujung approved these changes May 5, 2022

View reviewed changes

emiliom merged commit ee4360b into OSOceanAcoustics:dev May 5, 2022

emiliom mentioned this pull request May 5, 2022

Add echopype software and version provenance attributes to Sv and MVBS datasets #618

Closed

emiliom deleted the svmvbs_prov branch December 24, 2022 03:06

emiliom mentioned this pull request May 25, 2023

Replace use of custom provenance attributes with history attribute #1050

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize echopype software provenance and apply to Sv and MVBS #621

Generalize echopype software provenance and apply to Sv and MVBS #621

emiliom commented Apr 10, 2022 •

edited

Loading

codecov-commenter commented Apr 10, 2022 •

edited

Loading

leewujung Apr 12, 2022

emiliom Apr 12, 2022

leewujung May 5, 2022 •

edited

Loading

emiliom May 5, 2022 •

edited

Loading

leewujung May 5, 2022

emiliom commented Apr 21, 2022

emiliom commented Apr 21, 2022

emiliom commented Apr 28, 2022

leewujung commented Apr 28, 2022

emiliom commented May 1, 2022 •

edited

Loading

leewujung left a comment

emiliom commented May 5, 2022

leewujung commented May 5, 2022 •

edited

Loading

emiliom commented May 5, 2022

Generalize echopype software provenance and apply to Sv and MVBS #621

Generalize echopype software provenance and apply to Sv and MVBS #621

Conversation

emiliom commented Apr 10, 2022 • edited Loading

codecov-commenter commented Apr 10, 2022 • edited Loading

Codecov Report

leewujung Apr 12, 2022

Choose a reason for hiding this comment

emiliom Apr 12, 2022

Choose a reason for hiding this comment

leewujung May 5, 2022 • edited Loading

Choose a reason for hiding this comment

emiliom May 5, 2022 • edited Loading

Choose a reason for hiding this comment

leewujung May 5, 2022

Choose a reason for hiding this comment

emiliom commented Apr 21, 2022

emiliom commented Apr 21, 2022

emiliom commented Apr 28, 2022

leewujung commented Apr 28, 2022

emiliom commented May 1, 2022 • edited Loading

leewujung left a comment

Choose a reason for hiding this comment

emiliom commented May 5, 2022

leewujung commented May 5, 2022 • edited Loading

emiliom commented May 5, 2022

emiliom commented Apr 10, 2022 •

edited

Loading

codecov-commenter commented Apr 10, 2022 •

edited

Loading

leewujung May 5, 2022 •

edited

Loading

emiliom May 5, 2022 •

edited

Loading

emiliom commented May 1, 2022 •

edited

Loading

leewujung commented May 5, 2022 •

edited

Loading