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

refactor: change freq_norm from string to enum #248

Merged
merged 17 commits into from
Oct 27, 2023

Conversation

anujsinha3
Copy link
Contributor

@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

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

Comparison is base (57f5e09) 62.33% compared to head (b2f06d6) 62.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   62.33%   62.80%   +0.46%     
==========================================
  Files          33       33              
  Lines        3876     3890      +14     
  Branches      534      534              
==========================================
+ Hits         2416     2443      +27     
+ Misses       1336     1323      -13     
  Partials      124      124              
Files Coverage Δ
src/noisepy/seis/datatypes.py 93.15% <100.00%> (+0.12%) ⬆️
src/noisepy/seis/noise_module.py 33.60% <50.00%> (+0.68%) ⬆️
tests/test_whiten.py 77.45% <92.30%> (+10.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@carlosgjs carlosgjs left a comment

Choose a reason for hiding this comment

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

Looking great, couple of small corrections. Also, the failure in the notebook is a bit cumbersome to debug, but here's how to find the log:

image

In this case, the error is:

�[0;31mValidationError�[0m: 1 validation error for ConfigParameters
freq_norm
  Input should be a valid string [type=string_type, input_value=<FreqNorm.RMA: 'rma'>, input_type=FreqNorm]
    For further information visit https://errors.pydantic.dev/2.3/v/string_type

The ConfigParameter.freq_form field was still declared as str, needs to be updated to FreqNorm.

Also note that you can open the notebook in VS Code and run it directly there and the error should show up.

script/test_whiten.py Outdated Show resolved Hide resolved
src/noisepy/seis/datatypes.py Outdated Show resolved Hide resolved
@anujsinha3 anujsinha3 marked this pull request as draft October 25, 2023 22:57
@@ -158,12 +166,14 @@ def plot_2d(white_original, white_new):


# Use wrappers since test functions are not supposed to return values
def test_whiten1d():
_, _ = whiten1d()
@pytest.mark.parametrize("freqNorm", [FreqNorm.PHASE_ONLY, FreqNorm.RMA])
Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice! And I see that the code coverage check is now passing.

@anujsinha3
Copy link
Contributor Author

@carlosgjs: I am unable to find the logs for the notebook failure.

I am getting the below warning-error logs from the build pages log:
https://github.com/noisepy/NoisePy/actions/runs/6647272604/job/18062357031?pr=248#step:4:285

Also, I am not able to find the artifact log files as well. The picture you posted looks a bit distorted. Can you please clarify once again?

Set navigation_with_keys to False to get rid of the warning. Setting it to True has negative accessibility implications: pydata/pydata-sphinx-theme#1492
Set navigation_with_keys to True
Copy link
Collaborator

@carlosgjs carlosgjs 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, thanks!

@anujsinha3 anujsinha3 temporarily deployed to github-pages October 27, 2023 18:15 — with GitHub Actions Inactive
@anujsinha3 anujsinha3 temporarily deployed to github-pages October 27, 2023 18:40 — with GitHub Actions Inactive
@anujsinha3 anujsinha3 marked this pull request as ready for review October 27, 2023 18:46
@carlosgjs carlosgjs merged commit b08fe9d into noisepy:master Oct 27, 2023
25 checks passed
carlosgjs added a commit that referenced this pull request Oct 27, 2023
* refactor: change freq_norm from string to enum

* refactor: pre-commit hook format changes

* refactor: change freq_norm from string to enum

* refactor: correct import

* refactor: add parameterized test cases

* refactor: fix import

* refactor: fix variable type

* refactor: fix data sphinx error

* refactor: revert the change

* refactor: change initialization of freq_norm from string to enum

* refactor: change initialization of freq_norm from string to enum

* refactor: change initialization of freq_norm from string to enum

* refactor: Fix navigation_with_keys warning.

Set navigation_with_keys to False to get rid of the warning. Setting it to True has negative accessibility implications: pydata/pydata-sphinx-theme#1492

* refactor: Fix navigation_with_keys warning.

Set navigation_with_keys to True

* refactor: revert change

* refactor: set navigation_with_keys as false to prevent sphinx validation error

* refactor: style consistency with \n

Co-authored-by: anujsinha3 <[email protected]>
mdenolle added a commit that referenced this pull request Oct 27, 2023
refactor: change freq_norm from string to enum (#248)
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