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

Fixing pickling of AutoReject objects #193

Merged
merged 3 commits into from Dec 15, 2020

Conversation

hubertjb
Copy link
Contributor

This PR fixes issue #192.

- simplifying read_auto_reject to instantiate default AutoReject object before setting state
- adding pickling to io tests
@jasmainak
Copy link
Member

Looks like the CI fails due to an unrelated problem. But we can't merge the PR with CIs failing. Are you able to reproduce the problem locally on your machine? I can't seem to reproduce with MNE master ...

@jasmainak
Copy link
Member

also don't forget to update whats_new !

@hubertjb
Copy link
Contributor Author

@jasmainak weird, can't reproduce that either on my side. I can investigate a bit more tomorrow.

@jasmainak
Copy link
Member

Thank you so much!

@hubertjb
Copy link
Contributor Author

Quick update: after a little more digging, it looks like the error happens when mne tries to convert the default color values to be used passed by autoreject. In the stable version, there used to be a check before trying to convert the color value to an RGBA value: https://github.com/mne-tools/mne-python/blob/4944aa28c5abdad44cdab2293955fd73e29da683/mne/viz/epochs.py#L1185

In mne-master though, there is no check before the conversion, and so the function tries to convert None to an RGBA value:
https://github.com/mne-tools/mne-python/blob/ef2a2e491763db89d7411eee4c0dd49b1c8032ee/mne/viz/_figure.py#L2037

@jasmainak
Copy link
Member

woops, looks like I missed @drammock's comment between my emails. @drammock is there an easy fix for this?

@codecov-io
Copy link

codecov-io commented Dec 15, 2020

Codecov Report

Merging #193 (e28f688) into master (67dd5a4) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage   94.80%   94.89%   +0.08%     
==========================================
  Files           5        5              
  Lines         905      901       -4     
==========================================
- Hits          858      855       -3     
+ Misses         47       46       -1     
Impacted Files Coverage Δ
autoreject/autoreject.py 95.51% <100.00%> (+0.14%) ⬆️
autoreject/utils.py 93.69% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67dd5a4...e28f688. Read the comment docs.

@agramfort agramfort merged commit dd09424 into autoreject:master Dec 15, 2020
@agramfort
Copy link
Collaborator

Thx @hubertjb

@hubertjb
Copy link
Contributor Author

Thx @hubertjb

That was an efficient fix. Thanks @agramfort and @jasmainak !

@hubertjb hubertjb deleted the fixed-pickling branch December 15, 2020 13:59
@jasmainak
Copy link
Member

Thanks @hubertjb ! :)

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.

None yet

4 participants