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

Add Missing Fields to RawEEGLAB and EpochsEEGLAB Classes Using DataContainerEEGLAB #12773

Closed
wants to merge 11 commits into from

Conversation

MohammadMow
Copy link

Reference issue

Example: Fixes #12772.

What does this implement/fix?

Explain your changes.
This pull request adds new fields to the DataContainerEEGLAB class to ensure all relevant fields from an EEGLAB .set file are properly stored. This enhancement aims to improve the completeness and usability of the class.

Changes

  • Added attributes to the DataContainerEEGLAB class to store additional fields from the EEGLAB .set file.
  • Updated the RawEEGLAB and EpochsEEGLAB classes docstring to reflect the new attributes.

New Attributes

  • setname
  • filename
  • filepath
  • subject
  • group
  • condition
  • session
  • comments
  • nbchan
  • trials
  • pnts
  • srate
  • xmin
  • xmax
  • times
  • data
  • icaact
  • icawinv
  • icasphere
  • icaweights
  • icachansind
  • chanlocs
  • urchanlocs
  • chaninfo
  • ref
  • event
  • urevent
  • eventdescription
  • epoch
  • epochdescription
  • reject
  • stats
  • specdata
  • specicaact
  • splinefile
  • icasplinefile
  • dipfit
  • history
  • saved
  • etc
  • run
  • roi
  • datfile

Proposed Solution

Using the composition design pattern, the DataContainerEEGLAB class encapsulates an instance of the Bunch object containing the EEG data. This approach ensures modularity and clean organization of code, enhancing maintainability and readability.

Additional information

Any additional information you think is important.

Copy link

welcome bot commented Aug 2, 2024

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

@larsoner
Copy link
Member

larsoner commented Aug 2, 2024

I am not totally sold on this idea. I think the goal of the ICA EEGLAB reader is to get EEGLAB-computed ICA to conform to the same conventions / use cases as the MNE-Python computed ICA. This PR doesn't adhere to this, instead adding a bunch of EEGLAB-specific stuff.

I think rather than attaching this to the object, I'd rather have an Examples section in the docstring or an actual examples/preprocessing/<something>.py that shows people how to use pymatreader directly to access all these additional, non-MNE-style attributes.

@MohammadMow
Copy link
Author

Thank you for your feedback on the proposed changes. I appreciate your concerns about maintaining consistency with MNE-Python conventions and your suggestion to provide an example for users to access EEGLAB-specific attributes directly using pymatreader.

However, I believe that integrating EEGLAB-specific attributes directly into the DataContainerEEGLAB class offers several significant benefits that enhance the overall usability and efficiency of the MNE-Python framework. This changes may improve integration, by incorporating these attributes directly into the DataContainerEEGLAB class, we create a more seamless and integrated experience for users working with both MNE and EEGLAB. This approach allows users to access all relevant information within a single object, streamlining workflows and reducing the need to switch between different data structures and also users benefit from having immediate access to both MNE-style and EEGLAB-specific attributes without the need for additional code to extract these fields. This not only saves time but also minimizes the potential for errors, thereby increasing productivity and user satisfaction.

@scott-huberty
Copy link
Contributor

Disclaimer: I am not a reviewer of this PR.

But I just want to highlight, in addition to what Eric mentioned, that Raw/Epochs/Evoked classes aren't really supposed to have these type of extra/non-standard attributes, and they won't survive an I/O round trip. This has come up a few times.

@mscheltienne
Copy link
Member

I'm -1 on this, it's purely a container-class which return a subselection of the EEGLAB fields loaded by data = pymatreader.read_mat("fname.set") which returns all the EEGLAB fields.

It adheres with non of MNE's standards and interact with non of MNE's function.

However, if our current readers are missing some relevant fields from the .set file, it would be great to correct this and to parse those fields in their respective MNE object, as for instance mne.preprocessing.read_ica_eeglab is doing for ICA fields.

@drammock
Copy link
Member

drammock commented Aug 8, 2024

I'm -1 on this, it's purely a container-class which return a subselection of the EEGLAB fields loaded by data = pymatreader.read_mat("fname.set") which returns all the EEGLAB fields.

I agree with @mscheltienne here. If there are specific fields/attributes in the EEGLAB object that don't have an equivalent in the MNE objects created by our read_*_eeglab reader functions, and you can explain why you actually need that information to achieve a particular analysis goal, then I'm interested. But as @mscheltienne says, I don't really see the value in providing an object wrapper around the output of pymatreader.read_mat("fname.set").

Closing, but if you do in fact have a concrete use case where you need some of the info we currently don't include, feel free to re-open.

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.

Add Missing Fields to RawEEGLAB and EpochsEEGLAB Classes
5 participants