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

implemented MAD using scipy #153

Merged
merged 4 commits into from
Aug 24, 2024
Merged

Conversation

Ayush-Devs
Copy link
Contributor

PR Description:

this PR closes #149.
median_abs_deviation from scipy.stats is used to compute the MAD.

Merge Checklist

  • the PR has been reviewed and all comments are resolved
  • all [CI][what-is-ci] checks pass
  • (if applicable): the PR description includes the phrase closes #<issue-number> to [automatically close an issue][auto-close-documentation]
  • (if applicable): bug fixes, new features, or [API][what-is-api] changes are documented in [whats_new.rst][whats-new-file]
  • (if applicable): new contributors have added themselves to the authors list in the CITATION.cff file

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, but the idea would be to replace our custom function _mad with the scipy implementation, instead of just calling the scipy function from within _mad. Would you be up for implementing this?

@Ayush-Devs
Copy link
Contributor Author

just confirming before making the changes, you want me to remove the _mad function and directly implement scipy in the areas _mad function is called. Am I going in the right direction?

@sappelhoff
Copy link
Owner

yes, and resolving this issue would also mean checking that the tests all still pass, and that _mad is adequately and fully replaced everywhere.

@Ayush-Devs
Copy link
Contributor Author

Ayush-Devs commented Aug 24, 2024

The custom implementation of _mad raised a value error when the input was not in 1D or 2D. Test_utils is expecting a value error that is not thrown by scipy.stats.median_abs_deviation. How do you really want me to fix this issue?

  1. Remove the test that expects the value error:
    from test_utils.py line 166
    //Test exception with > 2-D arrays
    tst = np.random.rand(3, 3, 3)
    with pytest.raises(ValueError):
    median_abs_deviation(tst, axis=0)

  2. create a custom function (i already did that in my first commit)

  3. check the dimensions before the function is called.

i don't really understand the consequences of omitting this check for 3d arrays. please tell me how to proceed

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I think we could also leave the changes as they are. But the current test suite needs to pass.

pyprep/utils.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Aug 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.39%. Comparing base (82390ac) to head (5cd26a5).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
- Coverage   99.39%   99.39%   -0.01%     
==========================================
  Files          14       14              
  Lines        1315     1313       -2     
==========================================
- Hits         1307     1305       -2     
  Misses          8        8              

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


🚨 Try these New Features:

@sappelhoff
Copy link
Owner

Great, thanks @Ayush-Devs could you please:

  1. add a changelog entry
  2. add yourself to the CITATION.cff file

Then we can merge this.

@Ayush-Devs
Copy link
Contributor Author

I really hope I correctly updated the citation and changelog files. thank you so much for guiding me through this experience.

@sappelhoff sappelhoff merged commit 6e5c369 into sappelhoff:main Aug 24, 2024
9 checks passed
@sappelhoff
Copy link
Owner

Thanks @Ayush-Devs 🥳

@Ayush-Devs Ayush-Devs deleted the new-branch branch August 25, 2024 04:44
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.

We should replace our own implementation of MAD by the one now available in scipy
2 participants