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.

Sign up for GitHub

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

Jump to bottom

Improve documentation and config validation of loose and depth parameters; drop support for loose=None #915

Merged
merged 10 commits into from
Apr 2, 2024
Merged

Improve documentation and config validation of loose and depth parameters; drop support for loose=None #915

merged 10 commits into from
Apr 2, 2024

Conversation

hoechenberger
Copy link
Member

@larsoner @SophieHerbst WDYT?

We currently don't support volumetric or mixed source spaces anyway, so I thought we could simplify things a little.

I was even considering entirely removing the loose and depth settings but then wasn't sure if some of you might be using them sometimes? Personally I never touch those.

Before merging …

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

@hoechenberger hoechenberger marked this pull request as ready for review March 30, 2024 13:23
@larsoner
Copy link
Member

Eventually we could pretty easily add support for volumetric source estimates I think. So I'm not sure it's worth getting rid of 'auto' just to potentially add it back. And I think loose and depth are both useful from time to time, especially when working with template / non-individualized MRIs (you often want loose=1.)

@SophieHerbst
Copy link
Collaborator

I agree about keeping the loose and depth parameters, we do use them sometimes.
No opinion about the 'auto' option, but if mixed source spaces can be implemented easily, that would be useful.

@hoechenberger hoechenberger marked this pull request as draft April 2, 2024 08:22
@hoechenberger hoechenberger changed the title Simplify inverse configuration Improve documentation of loose and depth parameters; drop support for loose=None Apr 2, 2024
@hoechenberger hoechenberger marked this pull request as ready for review April 2, 2024 08:36
@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 2, 2024 •
edited
Loading

@larsoner @SophieHerbst
I now changed things to basically only improve the documentation and config validation, except for one change: I removed support for loose=None, as this is equivalent to looe=0, and I always disliked we had two ways to achieve the same outcome.

This, of course, introduces an inconsistency with MNE, which allows for loose=None, but I can live with that…

Thoughts?

@hoechenberger hoechenberger changed the title Improve documentation of loose and depth parameters; drop support for loose=None Improve documentation and config validation of loose and depth parameters; drop support for loose=None Apr 2, 2024
@SophieHerbst
Copy link
Collaborator

I have always found the options for those parameters a bit confusing, so I am fine with removing the None.

@larsoner
Copy link
Member

larsoner commented Apr 2, 2024

Okay with me to remove None

@hoechenberger @larsoner
Update _config.py
87bf517 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
larsoner
larsoner approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Marking for merge when green, thanks in advance @SophieHerbst !

@larsoner larsoner merged commit 6288770 into mne-tools:main Apr 2, 2024
53 checks passed
@larsoner
Copy link
Member

larsoner commented Apr 2, 2024

Whoops wrong PR comment, thanks @hoechenberger !

@hoechenberger hoechenberger deleted the inverse-config branch April 2, 2024 16:33
larsoner added a commit to larsoner/mne-bids-pipeline that referenced this pull request Apr 16, 2024
@larsoner
Merge remote-tracking branch 'upstream/main' into ica
56befac 
* upstream/main:
  change default for info to use for inverse mne-tools#905 (mne-tools#919)
  Improve documentation and config validation of `loose` and `depth` parameters; drop support for `loose=None` (mne-tools#915)
  enhance documentation of caching, continuation of mne-tools#914 (mne-tools#918)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#917)
  Restructure configuration options documentation sections (mne-tools#914)
  Try to fix documentation deployment (mne-tools#913)
  Do not show `Annotated` types in configuration options documentation (mne-tools#911)
  Add number of subjects to grand-average report (cont'd) (mne-tools#910)
  MAINT: Ensure input changes cause output changes (mne-tools#904)
  Render type annotations in the documentation again (mne-tools#909)
@behinger behinger mentioned this pull request May 21, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoechenberger @larsoner @SophieHerbst