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

MAINT: DRY kwarg passing #746

Merged
merged 3 commits into from
Jun 26, 2023
Merged

MAINT: DRY kwarg passing #746

merged 3 commits into from
Jun 26, 2023

Conversation

larsoner
Copy link
Member

Before merging …

  • Changelog has been updated (docs/source/changes.md)

I went to work on #574 and the sheer volume of kwargs in get_config that must be used to import_*_data made it really hard to start from one of the steps/preprocessing/*.py files to add a new step. This PR is a no-op from the user standpoint but refactors to:

  1. _bids_kwargs function that returns some standard BIDS kwargs like cfg.space. Eventually we should use this everywhere probably but I'm just starting with steps/preprocessing for now.
  2. _import_data_kwargs function that gets the config params needed for reading raw data

Should be a lot more red than green here. I don't expect the first version to pass -- locally I've only run ds004229 -- but hopefully it won't require many more iterations to get right!

eeg_template_montage=config.eeg_template_montage,
fix_stim_artifact=config.fix_stim_artifact,
stim_artifact_tmin=config.stim_artifact_tmin,
stim_artifact_tmax=config.stim_artifact_tmax,
find_flat_channels_meg=config.find_flat_channels_meg,
find_noisy_channels_meg=config.find_noisy_channels_meg,
Copy link
Member Author

@larsoner larsoner Jun 26, 2023

Choose a reason for hiding this comment

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

For this function for example now it's trivial to see that the only params it actually needs (other than raw data loading params) are the find_*_channels_meg values (EDIT: plus extra_kwargs above)!

Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

I love it! Great work

@hoechenberger hoechenberger merged commit ec978f5 into mne-tools:main Jun 26, 2023
7 checks passed
@larsoner larsoner deleted the refactor branch June 26, 2023 23:07
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.

2 participants