-
Notifications
You must be signed in to change notification settings - Fork 33
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
detrend_correction #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @yjmantilla! I also like the way you solved the problems. I merely added some formatting suggestions for the code to conform to pep8 and pep257.
In this pull request I have only put back the original default of PREP, I haven't made the change to make the frac_bad a passed parameter.
A new pull request for adding that change would be welcome!
BTW: if this fixes issue #8 then you can include one of the following snippets in your pull request description:
to automatically close your issue once this PR is merged. |
also, please feel free to add yourself to the list of authors in https://github.com/sappelhoff/pyprep/blob/master/docs/whats_new.rst (at the bottom). Please follow the existing formatting. |
Co-Authored-By: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
So I committed your suggestions onto the branch, now it is saying that some checks failed. Reading a bit it seems it is because of the formatting but I don't really know how to solve that because it doesn't quite say what should exactly be corrected (or idk how to see that.). So if you know how to correct that I would be glad if you told me. I have two other more modifications I think would be important but I don't want to pollute this pull request because they don't quite have anything to do with the detrend, but just so you know they are:
|
@yjmantilla yes, I did some general maintenance and docs work yesterday to prepare the package for a next release once your improvements are all in. In that process I also added GitHub Actions to the test suite. The issues are indeed related to formatting. You can run When you push some new commits to this PR, please also push one to add yourself to the authors list https://github.com/sappelhoff/pyprep/blob/master/docs/whats_new.rst (see my comment: #9 (comment)) and under the |
how much slower? Did you perform some timing tests? In general, this solution does seem better than automatically subsampling though, I agree with you on that. Maybe this contribution that you plan could also allow us to drop the
separate PR welcome! |
Thanks! I'll be checking the formatting and the author thing the next week because in the current one I need to have my research proposal ready. I haven't perform rigorous timing tests but just noticed it. I will take that into account for the PR of the ransac. Regarding the consumption of RAM maybe psutil wont be required for ransac but I see another issue:
For some of the data I'm processing it needs 9gb, but I do recognize this data is sampled at 1000Hz with a duration of about 5 minutes and 58 channels so maybe that is quite big. For now I just included that if line_freqs = [ ] then to skip directly to the reference stage. |
alright, good luck with your research proposal then and see you next week :-) |
@yjmantilla how is the situation? Can you estimate when you will have time to work on this? |
Hi, Im still working in my research stuff. Should be less than 2 days more (still have classes, homework and stuff but thats the normal). I passed the black formatting, added the bug to whats_new and myself to the authors lists as you suggested. It says there is a conflict with the whats new but I guess those are from the obvious changes. Now, I dont know if the tests are now passed or not to see if there is anything left to correct in this particular pull request. Once this one is actually merged I will make pull requests for the other changes I mentioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I see the conflict :-/ can you resolve it?
I think all is according to what you said to me (conflict, black, whats_new, etc). Looking forward to your feedback. |
There is still a conflict with whats_new unfortunately. This is because when you started the PR, you got a copy of whats_new to your branch ... however, in the meantime I did changes to whats_new UPSTREAM (meaning: in sappelhoff/pyprep, the "master repository"). And you did not get these updates, ... instead you applied your own updates, and now YOUR updates are clashing with MY updates (which happen to be the current master state) one way to resolve the conflict would be:
Note: before you do any of this, do: Let me know if I should explain something in more detail. |
Sure, I will try all of this. Sorry for inconvenience and thanks for the explanation. I've never done this level of github hacking before haha. |
okay, then here are some more handy tips:
|
Co-Authored-By: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
So I did the steps to do the rebase; apparently there aren't conflicts anymore and it passed all the tests. The only thing weird is that there are now 33 files changed, the majority of them seem from the commits you did after I did the fork so maybe that is to be expected? or maybe I did it wrong (?). In any case I look forward to your feedback as always. |
I see 🤔 something must have gone wrong during rebasing. I can't really say WHAT the issue was, possibly it's a GitHub issue as well (see: https://github.community/t5/How-to-use-Git-and-GitHub/GitHub-pull-requests-showing-invalid-diff-for-already-merged/td-p/3000) ... but I can't say for sure. I took some time to write up the workflow. I repeat a lot of stuff, and I don't really know your Hope this helps! Workflow
Syncing your fork's
|
I followed the steps but just after the rebase of the branch it says:
I also just after syncing my fork master with your master I did a git push to my master Here is how the graph looks: Here master is d81c5cf Maybe at this point I should just try the easy way. Although Im curious, if you try to do the merge as it is doens't it show somehow that much of the commits are already done since after all it took them from the current master?? |
If it helps at all here is the log:
|
oh damn ... I have a typo in my guide -.- sorry @yjmantilla The issue is here:
--> instead, it should be |
yes, I am curious about that as well ... it might be a GitHub bug that a diff is shown ... although there is no diff 🤔 I think I have tortured you enough now and I'll try to squash and commit your PR. |
Yes, seems like some GitHub issue, the squash-and-merge commit of the PR was clean ... See for yourself by:
And the diff is exactly what you contributed :-) thanks a lot for the fix and also for patiently following git instructions! 🚀 |
also, I slightly changed your whatsnew entries, see: 0bec296 mainly to:
|
This pull request fixes #8 thus closes #8
Regarding Detrends
In find_noisy_channels:
On lines 34-35 of the current version a detrend is done on a trended copy of the raw data as follows:
self.EEGData = self.raw_mne.get_data(picks="eeg")
self.EEGData = removeTrend(self.EEGData, sample_rate=self.sample_rate)
Now on lines 153-154 of the function find_bad_by_nan_flat we have:
self.raw_mne.drop_channels(list(set(self.bad_by_nan + self.bad_by_flat)))
self.EEGData = self.raw_mne.get_data(picks="eeg")
This replaces the EEGData with a trended version again (the idea was to drop the unusable channels I think).
This replacement with a trended version destabilizes the ransac quite a lot.
Furthermore in reference.py:
Through lines 131-135 a detrend is done then NoisyChannels (find_noisy_channels main class) is used, this means the detrend is applied twice.
raw._data = removeTrend(raw.get_data(), sample_rate=self.sfreq)
noisy_detector = NoisyChannels(raw)
noisy_detector.find_all_bads(ransac=self.ransac)
Because of this I decided tu put a boolean to indicate whether to do the detrend internally or assume the signal is already detrended.
Regarding correlation criteria:
In find_noisy_channels frac_bad is initialized with 0.1 as follows:
def find_bad_by_correlation(self, correlation_secs=1.0, correlation_threshold=0.4, frac_bad=0.1):
In the original PREP this values was at 0.01, now I understand this change was made because as they say in the exampe:
While this makes sense for the example the default should be the one of the original PREP. More so it would be necessary to make the frac_bad an input parameter to find_noisy_channels to cover cases similar to the example.
In this pull request I have only put back the original default of PREP, I haven't made the change to make the frac_bad a passed parameter.