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

Create a global dataclass to hold set-up specific settings #338

Closed
toni-neurosc opened this issue Jun 17, 2024 · 3 comments
Closed

Create a global dataclass to hold set-up specific settings #338

toni-neurosc opened this issue Jun 17, 2024 · 3 comments

Comments

@toni-neurosc
Copy link
Collaborator

toni-neurosc commented Jun 17, 2024

So I was checking the new read_mne_data function, that takes line_noise as do many things around the package. As I mentioned in PR #329 it seems like line_noise is crowding a lot of function parameter lists for a thing that is only actually read in a single line of the entire codebase.

So far we're sticking to the principle that NMSettings should only hold settings concerning the desired data analysis, and that has the great advantage that these settings are portable and shareable between setups and experiments.

On the other hand, excluding some set-up/recording specific settings from NMSettings makes it so that we have some relevant settings that just float around the code and need to be passed all the time around functions, when in reality is never a necessity to specify them more than just once. Off the top of my head:

  • sfreq
  • line_noise
  • nm_channels: a bit less straight-forward, but the more I think about it, the more sense it makes to me. Channels are set-up with the experiments, as they depend on the equipment available and the number and type of electrodes, EEG sensors or whatever other instrumentation is available at the time of the experiment.

I was thinking we can create some kind of data structure (e.g. a Pydantic dataclass) that holds these kind of variables, and call it ExperimentSettings or similar. If maybe that's overkill, maybe we can just make line_noise global, default to 50 Hz and set up a utility function called set_line_noise.

If this is considered a non-issue and needs no addressing, or if my assumptions are mistaken, feel free to close the issue.

@timonmerk
Copy link
Contributor

It's definitely a good point @toni-neurosc! I see the reason behind it keeping all those information at a single place. Currently however I would focus on different issues, and keep it in mind for later. We should keep the issue however open imo

@toni-neurosc
Copy link
Collaborator Author

toni-neurosc commented Jun 18, 2024

Well I just realized that his might be a duplicate of #315 ... it's not exactly the same but deals with the same fundamental issue. Maybe the 2 issues can be merged?

@toni-neurosc
Copy link
Collaborator Author

Duplicate of #315

@toni-neurosc toni-neurosc marked this as a duplicate of #315 Jun 18, 2024
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

No branches or pull requests

2 participants