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

Fix issue 171 #172

Merged
merged 6 commits into from
Dec 8, 2023
Merged

Fix issue 171 #172

merged 6 commits into from
Dec 8, 2023

Conversation

kkappler
Copy link
Collaborator

@kkappler kkappler commented Nov 15, 2023

Notes are in issue #171.

Remaining things to be addressed:

  • modify @sample_rate.setter to update self._sample_rate
  • test_change_sample rate modified back to using the setter, rather than assigning self._sample_rate explicitly
  • Add direct assignment of self._sample_rate when it is packed in channel_metadata on init
    • as a dict
    • as an mt_metadata object

- remove unneeded call when data are provided to ChannelTS
- if self.ts is assigned, _update_xarray_metadata is called within
  the setter
If we are going to use the self_sample_rate property
introduced, then the setting of sample rate should be
done via self._sample_rate.

The sample_rate setter should be checked for coverage, and
possbly modified.

Will add notes to issue #171
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2023

Codecov Report

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

Comparison is base (484acb9) 58.88% compared to head (5f41cc9) 58.87%.

Files Patch % Lines
mth5/timeseries/channel_ts.py 86.66% 2 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   58.88%   58.87%   -0.01%     
==========================================
  Files         138      138              
  Lines       14896    14906      +10     
==========================================
+ Hits         8771     8776       +5     
- Misses       6125     6130       +5     
Flag Coverage Δ
tests 58.87% <87.50%> (-0.01%) ⬇️

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.

- sample_rate setter now updates self._sample_rate
- test now uses the setter instead of accessing channel_ts._sample_rate
- self._sample_rate is now set on __init__()
- because it can be None, dict, or mt_metadata.timeseries.Electric
mt_metadata.timeseries.Magnetic, or mt_metadata.timeseries.Auxiliary
and the dict can be flat or nested, a little bit of care was needed
- created a method get_sample_rate_supplied_at_init() of ChannelTS to
  handle this
@kkappler
Copy link
Collaborator Author

kkappler commented Nov 30, 2023

@kujaku11 This PR is ready for review.

ChannelTS now has a self._sample_rate attribute.

This supports explicit assignment of sample_rate at __init__ via the channel_metadata kwarg.
Because channel_metadata can be None, dict, mt_metadata.timeseries.Electric, mt_metadata.timeseries.Magnetic, or mt_metadata.timeseries.Auxiliary and furthermore, if a dict it can be nested (e.g. channel_metadata={"electric":{"sample_rate":8.0}}) some logic was needed to handle this ... tha logic was placed in a new method of ChannelTS, called get_sample_rate_supplied_at_init.

The result when working with high frequency data is a significant speedup as the sample_rate attr is computed only once if not supplied.

@kkappler kkappler requested a review from kujaku11 November 30, 2023 16:35
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.

Looks more efficient

@kujaku11 kujaku11 merged commit 8d886af into master Dec 8, 2023
8 checks passed
@kujaku11 kujaku11 deleted the fix_issue_171 branch December 8, 2023 22:55
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.

3 participants