Closed scott-huberty closed 1 year ago
Note that in https://github.com/mne-tools/mne-icalabel/pull/130 , ICALABEL changed the name of their constant dict from ICLABEL_LABELS_TO_MNE
to ICA_LABELS_TO_MNE
, which is part of the latest stable released just yesterday (0.5), thus breaking our tests today.
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 mne.utils.check_version
to our package so that in the event that someone on our team doesnt have at least 0.5, they will get an informative error.
Merging #145 (f66e251) into main (dbb775e) will increase coverage by
1.18%
. The diff coverage is90.10%
.
@@ 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
Files | Coverage Δ | |
---|---|---|
pylossless/__init__.py | 100.00% <100.00%> (ø) |
|
pylossless/config/__init__.py | 100.00% <100.00%> (ø) |
|
pylossless/config/config.py | 91.30% <100.00%> (ø) |
|
pylossless/conftest.py | 100.00% <100.00%> (ø) |
|
pylossless/datasets/__init__.py | 100.00% <100.00%> (ø) |
|
pylossless/dash/__init__.py | 83.33% <80.00%> (-16.67%) |
:arrow_down: |
pylossless/pipeline.py | 75.56% <96.00%> (+1.22%) |
:arrow_up: |
pylossless/datasets/datasets.py | 92.30% <92.30%> (ø) |
|
pylossless/config/rejection.py | 72.22% <72.22%> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
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
Config
andRejectionPolicy
classes really seem to be two YAML compatible configuration files for applying pipeline procedures. HoweverRejectionPolicy
lacked 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.Config
is just a recipe that gets passed to theLosslessPipeline
class, whileRejectionPolicy
is 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 #138 ,
RejectionPolicy.apply()
is sleek, but it is a bit confusing per my point above, and becauseapply
suggests that we are changing some object directly, but really this method makes a new clean copy and returns it. On this branch,RejectionPolicy.Apply
is replaced byLosslessPipeline.make_cleaned_raw
which is hopefully more clear that we are returning a new object.I was/am a little concerned about the default
copy
every 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.