FIX, DOC: Pipeline methods shouldn't accept just any instance of raw #213

scott-huberty · 2024-12-16T18:16:52Z

Following up from #212. I realized a couple things...

First of all, isinstance(inst, mne.io.Raw) is still wrong. It should have been isinstance(inst, mne.io.BaseRaw).

More importantly, I don't think that we should actually support passing a raw object to this method...

Because for example, if one does my_pipeline.find_outlier_chs(my_raw), then under the hood this method actually creates an epochs instance from my_pipeline.raw (via pipeline.get_epochs()), and uses that in outlier detection ...

This would lead to unexpected results if my_raw and my_pipeline.raw are not the same object in memory (i.e. they are different raw objects).

Since in our codebase we never do pipeline.find_outlier_chs(raw), I dont think we should support this.

Instead, I think that always expecting an instance of mne.Epochs is OK.

Marking this PR as draft because I need to make sure the new docstring renders OK, but Opening the PR now so that others have time to comment.

Following up from lina-usc#212. I realized that if one did do `my_pipeline.find_outlier_chs(my_raw), then under the hood this method would actually create an epochs instance from my_pipeline.raw ... this would lead to unexpected results if my_raw and my_pipeline.raw are not the same object of memory (i.e. they are different raw objects). Since in our codebase we never do pipeline.find_outlier_chs(raw), I dont think we should support this. Instead, we should either always expect an instanc of mne.Epochs OR We should change the method signature to be def find_outlier_chs(epochs=None) , where if it is None, the method creates epochs from pipeline.raw under the hood.

codecov · 2024-12-16T18:31:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.43%. Comparing base (1297027) to head (c03a3fb).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   83.27%   83.43%   +0.15%     
==========================================
  Files          26       26              
  Lines        1698     1696       -2     
==========================================
+ Hits         1414     1415       +1     
+ Misses        284      281       -3

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

pylossless/pipeline.py

christian-oreilly

If you agree with these proposed changes, then any place in the pipeline where we us self.find_outlier_chs(epochs) could be replace by self.find_outlier_chs().

pylossless/pipeline.py

christian-oreilly · 2024-12-18T13:59:49Z

I think this method was probably written that way when we were still passing a raw object around instead of making it an attribute of the class and we failed to see that this change was breaking this functionality. But you are right that this code is currently broke insofar as the raw object passed as an argument is not used for the processing. We should then rename inst and epochs. We could also used a None default values like this

def find_outlier_chs(self, epochs, picks="eeg"):

    [...]

    if epochs is None:
        epochs = self.get_epochs(rereference=False, picks=picks)

- Change API form `inst` to `epochs | None` - if `None`, then make epochs from self.raw - No need check and raise type error now. Co-authored-by: Christian O'Reilly <[email protected]>

christian-oreilly · 2024-12-20T22:04:46Z

pylossless/pipeline.py

-            raise TypeError(
+        if epochs is None:
+            epochs = self.get_epochs(rereference=False, picks=picks)
                "inst must be an instance of mne.Epochs," f" but got {type(inst)}."


Oups! I think I left a line that was meant to be removed!

Suggested change

"inst must be an instance of mne.Epochs," f" but got {type(inst)}."

Already fixed in 2a747c2 🙂

scott-huberty · 2024-12-20T22:30:10Z

Alright @christian-oreilly thanks for the review, I think your suggestions made things a lot nicer.

FYI please do see 56a2242, I added a line to ensure that we call pick on the epochs regardless of whether we create epochs with self.get_epochs or if the user passes epochs in themselves.

Before this change, if the user passes in their own epochs, then pick would never be called..

scott-huberty · 2024-12-21T00:08:34Z

If you agree with these proposed changes, then any place in the pipeline where we us self.find_outlier_chs(epochs) could be replace by self.find_outlier_chs().

Oops I missed this part. Sure I’m fine with this, would just need to change the function signature to epochs=None so that you can do self.find_outlier_chs()

…ina-usc#213) * FIX, DOC: Pipeline methods shouldn't accept just any instance of raw Following up from lina-usc#212. I realized that if one did do `my_pipeline.find_outlier_chs(my_raw), then under the hood this method would actually create an epochs instance from my_pipeline.raw ... this would lead to unexpected results if my_raw and my_pipeline.raw are not the same object of memory (i.e. they are different raw objects). Since in our codebase we never do pipeline.find_outlier_chs(raw), I dont think we should support this. Instead, we should either always expect an instanc of mne.Epochs OR We should change the method signature to be def find_outlier_chs(epochs=None) , where if it is None, the method creates epochs from pipeline.raw under the hood. * Apply suggestions from Christian O'Reilly code review - Change API form `inst` to `epochs | None` - if `None`, then make epochs from self.raw - No need check and raise type error now. Co-authored-by: Christian O'Reilly <[email protected]> * FIX: cruft * DOC: improve docstring a little * DOC, FIX: add example to docstring and explicitly call pick * TST: OK codecov here is your test :) * STY: 2 lines bt functions * API: make epochs=None the default find_outlier_chs(epochs=None) --------- Co-authored-by: Christian O'Reilly <[email protected]>

scott-huberty requested a review from christian-oreilly December 16, 2024 18:16

scott-huberty marked this pull request as draft December 16, 2024 18:17

christian-oreilly reviewed Dec 18, 2024

View reviewed changes