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 235 #236

Open
wants to merge 41 commits into
base: patches
Choose a base branch
from
Open

Fix issue 235 #236

wants to merge 41 commits into from

Conversation

kkappler
Copy link
Collaborator

@kkappler kkappler commented Dec 23, 2024

Factored a TimeSeriesDecimation metadata class out of both FCDecimation and AuroraDecimationLevel. This will help future updates disambiguate the term decimation and is a general step towards adding Spectrogram capabilities into MTH5.

  • minor updates to nomenclateure and doc (Channel renamed to FCChannel to disambiguate from time series Channel)

- this will help disambiguate from time series Channel objects
- minor updates to doc strings
- remove an unneeded return None
- Add some typehinting for DecimationLevel
- working on finding if we can delete Decimation class
- once correctly dtyped, circular imports were encountered.
- this was fixed by creating frequency_bands.py
- also add FrequencyBands to __init__.py
- alphabetize imports in __init__.py
@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.84967% with 14 lines in your changes missing coverage. Please review.

Project coverage is 84.94%. Comparing base (ba2323e) to head (083e2aa).
Report is 35 commits behind head on patches.

Files with missing lines Patch % Lines
...fer_functions/processing/aurora/frequency_bands.py 82.85% 6 Missing ⚠️
...ions/processing/fourier_coefficients/decimation.py 92.00% 4 Missing ⚠️
...er_functions/processing/aurora/decimation_level.py 85.00% 3 Missing ⚠️
tests/tf/processing/test_time_series_decimation.py 90.90% 1 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##           patches     #236      +/-   ##
===========================================
- Coverage    84.98%   84.94%   -0.04%     
===========================================
  Files          270      273       +3     
  Lines        20515    20378     -137     
===========================================
- Hits         17435    17311     -124     
+ Misses        3080     3067      -13     
Flag Coverage Δ
tests 84.94% <90.84%> (-0.04%) ⬇️

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.

- move get_fft_harmonics into window.py from decimation_level.py
- add fft_harmonics method to Window and replace other imports and usages
- try to group the decimation-related attributes together at the top, and the STFT related attrs at the bottom
- this may not be needed and this json is slated for deletion, but
until ready to delete this keeps the dual decimation classes close
- this is one of two main replacements that needs to be done on #235
- the next main replacement will be in FCDecimation
- add pass-through getters and setters for decimation_level and decimation_factor,
  - allowing FCDecimation to assing/access via self.time_series_decimation
- remove attrs decimation_level and decimation_factor from fc decimation.json
- update attrs in time_series_decimation.json
- replace with a property in DecimationLevel that accesses TimeSeriesDecimation attribute.
@kkappler kkappler marked this pull request as ready for review December 28, 2024 00:36
@kkappler kkappler requested a review from kujaku11 December 28, 2024 00:36
@kkappler
Copy link
Collaborator Author

@kujaku11 This is ready for review. I cannot test aurora on github by changing the mt_metadata branch, but tests are passing locally for this. Also not sure how to test mth5 against this branch, but the integrated tests pass locally.

- remove some cruft
- add TODO and some doc
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.

2 participants