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: use meters for pupil SI, not moles #12850

Merged
merged 6 commits into from
Sep 18, 2024

Conversation

scott-huberty
Copy link
Contributor

fixes #12849

I actually need to simulate some pupil data in meters and test this plotting function to see if the scalings make sense (we probably need to adjust them).

@scott-huberty scott-huberty marked this pull request as draft September 13, 2024 20:19
@scott-huberty
Copy link
Contributor Author

OK, trying to establish some good scalings. If I simulate some pupil data ranging from 0 - 8mm and plot, I get this:

sfreq = 100
ch_names = ["my_pupil"]
ch_types = "pupil"
info = mne.create_info(ch_names, sfreq, ch_types)
data = np.full((1, 900), np.repeat(np.linspace(0, .008, 9), sfreq))
raw = mne.io.RawArray(data, info, verbose=False)
raw.plot(remove_dc=False)
Screenshot 2024-09-16 at 4 15 25 PM

@sappelhoff do you have the bandwidth to try this branch out with your eyetracking data?

@sappelhoff
Copy link
Member

Thanks, this seems to work very well!

image

there are two pupil dilation channels on top, and two "eye openness" channels at the bottom. The default scaling on this branch gets it right automatically, whereas on main, I need to manually adjust using + and -.

This is the same data window on main:

image

@scott-huberty
Copy link
Contributor Author

scott-huberty commented Sep 17, 2024

Ok awesome!

Working off the last MWE, here are the epoch and evoked plots:

import numpy as np
import mne

sfreq = 100
ch_names = ["my_pupil"]
ch_types = "pupil"
info = mne.create_info(ch_names, sfreq, ch_types)

baseline = np.repeat(6, sfreq / 10) # 100 ms

# Create a parabolic pupil constriction response

x = np.linspace(0, 2, sfreq * 2) # 2 seconds
a = 4 
h = 1
k = 2
y = a * (x - h) ** 2 + k
ep_data = np.concatenate([baseline, y])
ep_data /= 1000 # convert to meters

# 10 epochs, 1 channel, 200 samples
epochs = np.full((10, 1, len(ep_data)), ep_data)
epochs = mne.EpochsArray(epochs, info, tmin=-0.1)
epochs.plot()
epochs.average().plot()
Screenshot 2024-09-17 at 2 44 53 PM Screenshot 2024-09-17 at 2 44 39 PM

Which looks reasonable to me. I'm going to push an empty commit to trigger the CI's.

@scott-huberty scott-huberty changed the title FIX: use meters for pupil SI, not molars FIX: use meters for pupil SI, not moles Sep 17, 2024
@scott-huberty scott-huberty marked this pull request as ready for review September 17, 2024 22:39
doc/changes/devel/12850.bugfix.rst Outdated Show resolved Hide resolved
@sappelhoff
Copy link
Member

great, thanks scott!

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Copy link
Member

@drammock drammock Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a within-release bugfix, so a changelog entry is actually not needed (and even has the potential to cause slight confusion --- we don't want people unnecessarily worrying about wrong axes labels if they're upgrading from stable-to-stable versions, which is the assumption we make of our changelog readers). @scott-huberty once the CIs have finished running, please add no-changelog-entry-needed label, then delete this file and push with ci-skip in the commit msg (no need to re-run them for just a deleted changelog entry). Then ping me; I'll do a manual merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drammock done. However AFAICT I can't add any labels to this PR, you might have to do it on my behalf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drammock Also just a heads up that the pip-pre jobs didn't pass the last time the CI's ran. The failures were in the decoding module: https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=31286&view=logs&j=dded70eb-633c-5c42-e995-a7f8d1f99d91&t=1ccededa-7e7e-5a92-87ce-e531d760d025

@drammock
Copy link
Member

@drammock Also just a heads up that the pip-pre jobs didn't pass the last time the CI's ran. The failures were in the decoding module

yeah that's happening in #12860 too so I'll go ahead and merge this one, we can chase down the decoding-on-Windows problem over there.

@drammock drammock merged commit 4f58e81 into mne-tools:main Sep 18, 2024
5 checks passed
@scott-huberty scott-huberty deleted the pupil_scalings_fix branch September 18, 2024 17:37
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.

looking a bit late, but why is the unit for pupil µM instead of µm?? M is moles / molar
3 participants