neuromodulation / py_neuromodulation

Real-time analysis of intracranial neurophysiology recordings.
https://neuromodulation.github.io/py_neuromodulation/
MIT License
41 stars 9 forks source link

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

Open toni-neurosc opened 1 week ago

toni-neurosc commented 1 week ago

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:

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

Duplicate of #315