Rejection policy refactor #145
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tion-policy Merge and fix conflicts. Accepted the change in this PR, and will re format with black in follow up commit.
Ran the black style formatter on these files
read_raw_bids on this file will throw a warning about unit conversion. We generally want our CI's to fail on warnings, so we change the logging level in the read_raw_bids call to "ERROR" to suppress the warning in this specific case.
- Since applying RejectionPolicy returns a copy, make it more clear in the funcion name - Make Workflow consistent across configs. LosslessPipeline class is the machinery, Configs are the recipes. - Thus, now Config and Rejection Policy are created similar, and their saved filepaths must be passed to the pipeline method, make_cleaned_raw - the pipeline class handles returning a copy of a clean raw - Made the unit tests more complete. - added a tutorial that features a full pipeline run and rejection
in v.02 MNE-ICALABEL changed ICLABELS_to_MNE to ICA_Labels_to_MNE I dont think we need to triage between versions and can just pin to 0.2 or above
|
Note that in mne-tools/mne-icalabel#130 , ICALABEL changed the name of their constant dict from Because this is still a very small project I think it's okay to pin to mne-icalabel 0.5 or greater, and I added an |
Codecov Report
@@ Coverage Diff @@
## main #145 +/- ##
==========================================
+ Coverage 71.60% 72.79% +1.18%
==========================================
Files 18 21 +3
Lines 1074 1154 +80
==========================================
+ Hits 769 840 +71
- Misses 305 314 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #115
closes / supercedes #138
I've been working through #138 (which was a great start!) which led to some refactoring. Instead of pushing the changes to that branch (and over writing some of the initial work), I copied the branch and pushed it here so that we can discuss/compare if necesary.
I had some nitpicks that I've addressed here:
Config class and RejectionPolicy
The
ConfigandRejectionPolicyclasses really seem to be two YAML compatible configuration files for applying pipeline procedures. HoweverRejectionPolicylacked a constructor (or default file) that helps the user to know what fields there are, what can be set, etc.. I've added a constructor here, and API doc.Configis just a recipe that gets passed to theLosslessPipelineclass, whileRejectionPolicyis both the recipe and the machinery to make the recipe, which I think was confusing API wise. in this branch, both configs are just the recipes, and all the machinery remains with the pipeline class.on Adding the rejection policy. #138 ,
RejectionPolicy.apply()is sleek, but it is a bit confusing per my point above, and becauseapplysuggests that we are changing some object directly, but really this method makes a new clean copy and returns it. On this branch,RejectionPolicy.Applyis replaced byLosslessPipeline.make_cleaned_rawwhich is hopefully more clear that we are returning a new object.I was/am a little concerned about the default
copyevery time we apply the rejection policy (memory consumption), but I also can see your logic behind preserving the state of the pipeline and its raw object, so I left the default copy and if it becomes a problem in the future we can address it then.tests and tutorials
I built out the unit tests to be more comprehensive (there was a typo in #138 that wasn't being caught by the tests), and I've added a tutorial on running the pipeline and applying the Rejection Policy
I committed my changes directly on my local #138 branch then duplicated it into this branch for this PR. So if there are concerns about contributing authorship etc, we can push the changes onto that branch and merge there.