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

MRG: Fix bug in nirs merge data #8306

Merged
merged 1 commit into from Sep 25, 2020
Merged

Conversation

rob-luke
Copy link
Member

What does this implement/fix?

The _merge_nirs_data function is used in topo plotting and averages channels that are close together for a nicer display. After a channel is merged it is then removed from the data. If the same channel was merged twice, then it was removed twice, but the second time it was removing legitimate data. This would then mean the size of data and pos was incorrect and a nice error was thrown in the following topo function (which made it easy for met to find this bug). Here I just ensure each channel is only removed once.

Additional information

I think this is a candidate for back port. Whats involved in that?

I found this error because I am considering proposing a different distance for merging channels, but that's an enhancement and I will put in another PR. But it means others aren't likely to hit this bug unless they are playing with internal functions.

@rob-luke rob-luke changed the title WIP: Fix bug in nirs merging data code WIP: Fix bug in nirs merge data Sep 25, 2020
@agramfort
Copy link
Member

is the codecov issue legit?

@rob-luke
Copy link
Member Author

rob-luke commented Sep 25, 2020

Yeah we aren't hitting the lines. I just checked locally that if we make #8307 we will hit the lines.

@rob-luke
Copy link
Member Author

Im trying to mock hit the lines now. Real data would be better, but cant get in lab.

@agramfort
Copy link
Member

agramfort commented Sep 25, 2020 via email

@rob-luke
Copy link
Member Author

I will try for a bit more to make a test. I will ping you when ready (or if I give up)

@rob-luke
Copy link
Member Author

rob-luke commented Sep 25, 2020

Sorry but I cant make it hit the line with this 1e-10 parameter in #8307. I am very confident this is bug though. Happy to merge if you are

@agramfort
Copy link
Member

agramfort commented Sep 25, 2020 via email

@rob-luke rob-luke changed the title WIP: Fix bug in nirs merge data MRG: Fix bug in nirs merge data Sep 25, 2020
@drammock
Copy link
Member

drammock commented Sep 25, 2020

I think this is a candidate for back port. Whats involved in that?

Once this PR is merged, fetch latest upstream and update your local copy of master branch. Then create local branch that tracks upstream:maint/0.21, cherry-pick the relevant commit, and push to upstream maint/0.21. If you don't have push access to upstream, you can do it as a PR (push local maint/0.21 to your fork, and open a PR from rob-luke/mne-python:maint/0.21 into mne-tools/mne-python:maint/0.21)

@larsoner larsoner merged commit b2d759a into mne-tools:master Sep 25, 2020
3 checks passed
larsoner pushed a commit that referenced this pull request Sep 25, 2020
@larsoner
Copy link
Member

Backported

@rob-luke
Copy link
Member Author

Thanks @agramfort @larsoner @drammock

marsipu pushed a commit to marsipu/mne-python that referenced this pull request Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants