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

Replaced deprecated pick_types with raw.pick #154

Closed
wants to merge 5 commits into from

Conversation

Ayush-Devs
Copy link
Contributor

PR Description

closes #151
This PR:
changes all instances of pick_types with raw.pick.
removes "import mne" from pyprep/utils.py and pyprep/tests/test_prep_pipeline.py as they are no longer in use

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! The tests are failing after your changes - could you look into what is causing the issue, please?

pyprep/utils.py Outdated
@@ -2,7 +2,7 @@
import math
from cmath import sqrt

import mne
# import mne
Copy link
Owner

Choose a reason for hiding this comment

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

please just remove this line instead of commenting it out

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #154   +/-   ##
=======================================
  Coverage   99.39%   99.39%           
=======================================
  Files          14       14           
  Lines        1315     1330   +15     
=======================================
+ Hits         1307     1322   +15     
  Misses          8        8           

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


🚨 Try these New Features:

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.

Hey @Ayush-Devs, just to give you a bit of feedback: I am seeing a lot of changes that are unrelated to the purpose of the issue (replaacing pick_types with pick). This makes the diff hard to read to the reviewers, and it will also open your contribution up to criticisms that you may not want. For example I see that you have removed a docstring and replaced it with a one-liner.

I strongly recommend that you keep your changes as concise and small as possible, and not to touch any lines that are unrelated.

@Ayush-Devs
Copy link
Contributor Author

Yes sorry, I got a bit distracted in passing those tests. Thanks for the review, it will be fixed soon.

Apart from that, I was not very confident with the amount of changes I needed to do before commiting.
Is this normal or should I change my approach. I did try to minimize the changes but the tests kept on failing.

@sappelhoff
Copy link
Owner

Apart from that, I was not very confident with the amount of changes I needed to do before commiting.

It is generally better to do small but meaningful commits rather than big commits that mix a lot of things

I did try to minimize the changes but the tests kept on failing.

It is best to test locally and figure out the root cause of the failure, and then commit a small change that fixes precisely what needs fixing.

@Ayush-Devs Ayush-Devs closed this Aug 24, 2024
@Ayush-Devs Ayush-Devs deleted the second branch August 24, 2024 14:18
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.

Move from pick_types to inst.pick
2 participants