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

[ENH] Create functions to read Epochs and Evoked Neuroscan formats #12392

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

withmywoessner
Copy link
Contributor

Reference issue

#12367

What does this implement/fix?

Add functions to read .avg (Evoked) and .eeg (Epochs) Neuroscan files

@withmywoessner withmywoessner changed the title Create functions to read Neuroscan formats Create functions to read Epochs and Evoked Neuroscan formats Jan 28, 2024
@larsoner
Copy link
Member

@withmywoessner you wrote a comment that I got pinged on but it's gone, did you delete it? If so hopefully it's because it's resolved 🙂 Or maybe #12393 is related? In any case happy to help if needed

@withmywoessner withmywoessner changed the title Create functions to read Epochs and Evoked Neuroscan formats [ENH] Create functions to read Epochs and Evoked Neuroscan formats Feb 3, 2024
@withmywoessner
Copy link
Contributor Author

@larsoner Just curious, do you know why there is no BaseEvoked class? Is it just because its much simpler than the other data classes?

@agramfort
Copy link
Member

agramfort commented Feb 4, 2024 via email

@withmywoessner
Copy link
Contributor Author

There is @agramfort? I was just curious why Epochs and Raw both inherit from a BaseEpochs and BaseRaw class, but Evoked does not.

@larsoner
Copy link
Member

larsoner commented Feb 4, 2024

I think because evoked data are always preloaded we decided Evoked could just be the base for other classes like EvokedArray or EvokedMFF (if we have that class, can't remember). For epochs and raw which allow loading on the fly things are more complicated.

In principle we could have BaseEvoked and Evoked would inherit from that (and otherwise just be a pass). One way forward would be to make Evoked an alias for BaseEvoked, and EvokedFIF be what's returned by read_evokeds. I don't think this would cause code churn for users or even our code base. But I haven't thought about it thoroughly.

@withmywoessner
Copy link
Contributor Author

withmywoessner commented Feb 5, 2024

Thnaks! @larsoner So you do think I should make a CNTEvoked class that inherits from Evoked or just call the EvokedArray constructor in the reader? Most of the io file epochs reader functions (which don't have an evoked reader) call Epochs constructor which inherits from BaseEpochs. The exception to this is the FieldTrip module which calls the Evoked/Epochs/Raw-Array constructor in its reader functions.

@agramfort
Copy link
Member

agramfort commented Feb 5, 2024 via email

@withmywoessner
Copy link
Contributor Author

withmywoessner commented Feb 6, 2024

I had another question about how Evoked works @larsoner. The Neuroscan .avg Evoke-like files have a parameter for number of rejected epochs. I see that EvokedArray has a parameter in the constructor for nave (Number of averaged epochs.), but nothing for number of rejected. I would like to include the .avg rejected epochs in the EvokedArray somehow.

Questions:
Do you recommend I just put this in comment
(Comment on dataset. Can be the condition. Defaults to ‘’.) ?

Do you know if other evoked file readers deal with this? I didn't see any.

Also, this is probably unnecessary, but do you think it could be worthwhile to add a nreject parameter to the EvokedArray to include the number of rejected epochs at some point?

@withmywoessner
Copy link
Contributor Author

Also let me know if I am pinging too much. I never know if it's better just to post a reply without the ping.

@agramfort
Copy link
Member

agramfort commented Feb 7, 2024 via email

@drammock
Copy link
Member

drammock commented Feb 7, 2024

Also, this is probably unnecessary, but do you think it could be worthwhile to add a nreject parameter to the EvokedArray to include the number of rejected epochs at some point?

@withmywoessner Can you say more about why you want to preserve that information? If you loaded a Neuroscan .avg file into MNE, and the "number of rejected epochs" was available as an attribute of Evoked, how would you use that information? (I'm trying to determine if this is a niche or nice-to-have information, or if important aspects of your workflow might depend on knowing it). For example, nave is needed because it is used to scale the noise covariance when doing inverse imaging (it's a rough proxy for SNR).

@withmywoessner
Copy link
Contributor Author

@drammock I think I mainly just want it for quality control purposes to see how many epochs.

@drammock
Copy link
Member

drammock commented Feb 7, 2024

ok, in that case I would agree with @agramfort: put the info in the comment attribute

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.

4 participants