[MRG] fixed bugs in preproc.py #22
Conversation
annapasca
commented
Sep 10, 2018
annapasca
commented
- added ref_meg='auto'
- insert try for plot_psd
insert try for plot_psd
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 21.55% 29.19% +7.64%
==========================================
Files 18 19 +1
Lines 1429 1459 +30
==========================================
+ Hits 308 426 +118
+ Misses 1121 1033 -88
Continue to review full report at Codecov.
|
ephypype/preproc.py
Outdated
|
|
||
| report.add_figs_to_section(figs=psds, captions=captions_psd, | ||
| section='ICA - muscles') | ||
| except: # noqa |
There was a problem hiding this comment.
it's not a good idea to put try-except clause in your repo. It's better to do it with an if-else ... and raise an appropriate warning
There was a problem hiding this comment.
plot_psd raises an error. Looking at the MNE code it seems due to the fact that now to plot the PSD of 1 ICA component we have to use directly psd_multipaper. @dmalt do you have time to ckeck this?
@jasmainak the try was a temporary fix :)
|
@dmalt @jasmainak I removed the try and substitute the plot_psd with psd_multipaper + plot |
|
I added some unit test...could they be ok? |
|
@jasmainak it seems we need scikit-learn |
ephypype/preproc.py
Outdated
| psds.append(fig) | ||
| # fig = ica_src.plot_psd(tmax=60, picks=[i_ic], fmax=140, show=False) | ||
| # fig.set_figheight(3) | ||
| # fig.set_figwidth(5) |
There was a problem hiding this comment.
remove these comments when you are ready to merge
There was a problem hiding this comment.
and set PR to [MRG] when you are ready :)
There was a problem hiding this comment.
@annapasca you forgot to address this. But I'll do it for you now
|
@annapasca you will need to update |
|
Thanks @annapasca for adding tests. That is awesome !! 🎉 |
|
the error in appveyor it seems due to some download problem of MNE, is it? How can I start again appveyor? |
|
@annapasca you may want to add a tiny commit or amend the last commit and force push. I don't think there is a way to restart the build like in Travis ... |
|
@jasmainak can I restart travis? It says the build has terminated but no error specified |
|
@annapasca sure feel free to restart Travis if you need. But this error is specific. It says:
Your tests should ideally be fast so that the screen is updating all the time. 10 minutes is the upper limit ... |
@jasmainak thks! I understood now! I tried to make the test faster |
ephypype/preproc.py
Outdated
| psds.append(fig) | ||
| # fig = ica_src.plot_psd(tmax=60, picks=[i_ic], fmax=140, show=False) | ||
| # fig.set_figheight(3) | ||
| # fig.set_figwidth(5) |
There was a problem hiding this comment.
@annapasca you forgot to address this. But I'll do it for you now
ephypype/tests/test_preproc.py
Outdated
| def test_compute_ica(): | ||
| """Test compute ICA on raw data.""" | ||
| ECG_ch_name = 'ECG' | ||
| EoG_ch_name = 'HEOG, VEOG' |
There was a problem hiding this comment.
I'm surprised your pep8 linter didn't throw an error for this. Variable names should always be in lower case except in some limited cases.
|
also @annapasca try to give your feature branch a more meaningful name if you can :-) Otherwise it's hard to try two pull requests which have the same name as the feature branch ... |
|
Few more comments:
But I'd say not urgent for now. Let's raise an issue so we don't forget. |
|
@jasmainak about test
NeuroPycon works saving most output in the absolute path, thus how can we solve this issue for the test?
OK! How is possible to avoid this? Thks a lot for all your useful suggestions! :) |
|
@annapasca see here: https://github.com/mne-tools/mne-python/blob/master/mne/tests/test_epochs.py#L657 how to create a temporary directory. You can probably also use this directly. |
why don't you close the matplotlib figures once you use them in the report object? |
