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

FIX: blink annotations eyetracking/pupilometry #12415

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

schekroud
Copy link

Reference issue

Fixes #12408

What does this implement/fix?

properly adjusts blink onsets/offsets to account for the start time of the signal. This allows you to crop the signal and still have an accurate description of blink onsets

schekroud and others added 3 commits February 2, 2024 18:49
alter interpolation function to account for first sample of the raw signal - which allows you to account for if a raw object has been cropped to accurately interpolate the correct periods of data
import required annotations function properly
@schekroud
Copy link
Author

@scott-huberty @larsoner sorry if i have completely screwed up how you guys normally do this - i closed the previous PR (i dont think i could edit the code once the PR was made) as i noticed it didn't import the annotations function needed (i incorrectly assumed it would be there by default, but then tested and found i was wrong).

This should work!

Again, sorry if i have messed up the normal workflow 😆

@larsoner
Copy link
Member

larsoner commented Feb 2, 2024

@schekroud you can just edit the code in your schekroud:main branch on GitHub and those changes will be reflected in the PR. No need to close and open a new one

@scott-huberty
Copy link
Contributor

No worries @schekroud there can be a learning curve for PR's. Have you read the contributing guide? I still use it from time to time to remind myself.

One comment (@larsoner can correct me if it isn't a big deal in this case), usually it's best to open a PR from a development branch (not your main branch). This should be covered in the contributing guide (let us know if it isn't).

@scott-huberty
Copy link
Contributor

scott-huberty commented Feb 5, 2024

I just took a closer look at this PR. It looks like most of the failing tests are unrelated, and I think all of them should have been fixed by recently merged PR's (i.e. #12417 ).

@schekroud if you do want to work on this PR from a development branch here is what I would do:

git commands

(Assuming you are in the mne-python github respoitory directory, and on your main branch):

# note I have edited the commands below since initially posting this comment

# checkout new branch, bringing over commits from y9ou rmain branch
git checkout -b fix_interpolate_blinks

# add a link to the mne-python github repo, we'll call it "upstream"
git remote add upstream https://github.com/mne-tools/mne-python.git

# bring in the latest changes that have been merged into mne-python
git fetch upstream main
git merge upstream/main --ff-only

# push your new local development branch to your remote fork
 git push --set-upstream origin fix_interpolate_blinks

Then you could open another PR and close this one.

resetting your main branch to be in line with mne-pythons main branch

if you create a new branch with the changes from this PR, you'll want to reset your main branch:

git checkout main

git reset --hard upstream/main

Either way (you create a new dev branch or just continue this PR from your main branch), There a couple things to do.

  1. add a new rst file to doc/changes/devel that describe the changes made by this PR. See https://mne.tools/dev/development/contributing.html#describe-your-changes-in-the-changelog
  2. @larsoner should we amend test_interpolate_blinks to include a test with a cropped raw object?

@sappelhoff sappelhoff changed the title update pupillometry.py FIX: blink annotations eyetracking/pupilometry Feb 8, 2024
@scott-huberty
Copy link
Contributor

Hi @schekroud do you still have time to work on this? I think you just need to merge in the updates to mne since your last commit, and add an entry to the changelog (see point 1 in my previous message). Let me know if you need any support from us!

@mscheltienne
Copy link
Member

I merged main. A test should be added to cover the previous failure.

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.

raw.crop doesn't adjust onset of annotations for eyetracking data leading to interpolation errors
4 participants