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

add (failing) test that fc_metadata updates as expected #173

Merged
merged 22 commits into from
Dec 22, 2023

Conversation

kkappler
Copy link
Collaborator

The fc decimation level updates via the setter but the changes are not propagating to the h5, and when an fc_group returns metadata (that has been "written" and "updated") the values in the metadata are still the defaults -- not the values that were set.

Inside mth5/tests/version_1/test_fcs.py I placed
test_can_update_decimation_level_metadata
which highlights the issue.

@kkappler kkappler requested a review from kujaku11 November 23, 2023 01:25
kujaku11 added a commit that referenced this pull request Dec 15, 2023
- Fix issue #173 (filter logic: `units_in`, `units_out`,  and `applied`)
  - There is now a defined direction of filters
  - Filters are only for instrument response
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (8e00c74) 58.95% compared to head (c91e83d) 59.53%.

Files Patch % Lines
mth5/groups/base.py 58.06% 13 Missing ⚠️
tests/version_2/test_make_mth5.py 0.00% 6 Missing ⚠️
mth5/groups/station.py 54.54% 5 Missing ⚠️
mth5/clients/zen.py 0.00% 4 Missing ⚠️
mth5/timeseries/run_ts.py 20.00% 4 Missing ⚠️
mth5/clients/phoenix.py 0.00% 3 Missing ⚠️
mth5/clients/geomag.py 50.00% 2 Missing ⚠️
mth5/groups/fc_dataset.py 33.33% 2 Missing ⚠️
mth5/groups/survey.py 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   58.95%   59.53%   +0.57%     
==========================================
  Files         138      138              
  Lines       14997    15244     +247     
==========================================
+ Hits         8842     9076     +234     
- Misses       6155     6168      +13     
Flag Coverage Δ
tests 59.53% <86.57%> (+0.57%) ⬆️

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.

@kujaku11
Copy link
Owner

@kkappler Have a look at the fixes and see if it works for you. The issue was the metadata was there, but it was not being read in so it was returning the default values.

The fix was when calling group.get_other_group the metadata is directly read now. The tests pass, but be sure that it works for you before we approve the pull.

@kkappler
Copy link
Collaborator Author

@kujaku11 This now passes the tests in aurora (tests/synthetic/test_fourier_coefficients.py) with the addition of a try/except on write_metadata in case the file is read-only

@kujaku11 kujaku11 linked an issue Dec 21, 2023 that may be closed by this pull request
Copy link
Owner

@kujaku11 kujaku11 left a comment

Choose a reason for hiding this comment

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

Changes look good.

kkappler and others added 2 commits December 21, 2023 16:04
- Notebook updates change `channel_response_filter` to `channel_response`
Copy link
Owner

@kujaku11 kujaku11 left a comment

Choose a reason for hiding this comment

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

Fixes look good.

@kujaku11 kujaku11 merged commit 79828c5 into master Dec 22, 2023
10 checks passed
@kujaku11 kujaku11 deleted the aurora_issue_310 branch December 22, 2023 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change update_{name}_metadata to update_metadata
3 participants