Skip to content

[DOC] Updated decoding.SSD documentation and internal variable naming #11475

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
Merged
drammock merged 8 commits into from
Feb 16, 2023
Merged

[DOC] Updated decoding.SSD documentation and internal variable naming #11475

drammock merged 8 commits into from
Feb 16, 2023

Conversation

@tsbinns
Copy link
Contributor

@tsbinns tsbinns commented Feb 14, 2023

Implementation of comments/suggestions in merged PR #11458.

  • Fixed incorrect default values in the docstring.
  • Reworded the docstring to better reflect the behaviour of the class.
  • Changed the naming of internal variables for consistency and clarity.
  • Changed a logging message to more accurately reflect the behaviour of the code.

Cheers, Thomas!

@tsbinns
added author
88758ab 
Revert "added author"

This reverts commit b9f34fa.

Revert "Revert "added author""

This reverts commit da587a6.
@tsbinns
updated documentation
e735f6b
@tsbinns
updated variable naming
eb28cf5
@larsoner
Copy link
Member

larsoner commented Feb 15, 2023

@vpeterson can you look? Then we'll mark for merge when you're happy

larsoner
larsoner reviewed Feb 15, 2023
View reviewed changes
vpeterson
vpeterson reviewed Feb 16, 2023
View reviewed changes
mne/decoding/ssd.py
If set to True, the components are sorted according to the spectral
ratio.
See Eq. (24) in :footcite:`NikulinEtAl2011`.
return_filtered : bool (default False)
Copy link
Contributor

@vpeterson vpeterson Feb 16, 2023

Choose a reason for hiding this comment

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

reasons for changing these defaults?!

Copy link
Contributor Author

@tsbinns tsbinns Feb 16, 2023

Choose a reason for hiding this comment

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

In __init__, sort_by_spectral_ratio is default True, and return_filtered is default False:

mne-python/mne/decoding/ssd.py

Lines 86 to 89 in 4ee2300

def __init__(self, info, filt_params_signal, filt_params_noise,
reg=None, n_components=None, picks=None,
sort_by_spectral_ratio=True, return_filtered=False,
n_fft=None, cov_method_params=None, rank=None):

however the existing documentation lists this the other way around:

mne-python/mne/decoding/ssd.py

Lines 51 to 57 in 4ee2300

sort_by_spectral_ratio : bool (default False)
If set to True, the components are sorted accordingly
to the spectral ratio.
See Eq. (24) in :footcite:`NikulinEtAl2011`.
return_filtered : bool (default True)
If return_filtered is True, data is bandpassed and projected onto
the SSD components.

so this change was just to have the documentation reflect the actual default values.

mne/decoding/ssd.py Outdated
This controls the rank computation that can be read from the
measurement info or estimated from the data.
measurement info or estimated from the data, which determines the
number of available components.
Copy link
Contributor

@vpeterson vpeterson Feb 16, 2023

Choose a reason for hiding this comment

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

I would say "which determines the maximum possible number of components"

@drammock drammock enabled auto-merge (squash) February 16, 2023 16:30
@drammock drammock merged commit 0dea13c into mne-tools:main Feb 16, 2023
larsoner added a commit to drammock/mne-python that referenced this pull request Feb 22, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into test-mpl-fix
c065743 
* upstream/main: (46 commits)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  [MRG] Forward argument axes from plot_sensors to DigMontage.plot (mne-tools#11470)
  [MRG] Improve error message raised on channels missing from DigMontage (mne-tools#11472)
  MAINT: Deal with pkg_resources usage bugs (mne-tools#11478)
  Add object array support and docstring (mne-tools#11465)
  [ENH] Adjusted SSD algorithm to support non-full rank data (mne-tools#11458)
  [BUG] fix nibabel reference (mne-tools#11467)
  ...
larsoner added a commit to larsoner/mne-python that referenced this pull request Mar 1, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into feature/TD-nirs_snirf
15e2878 
* upstream/main: (264 commits)
  BUG: Fix deprecated API usage in example (mne-tools#11512)
  Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500)
  EGI/MFF events outside EEG recording should not break the code (mne-tools#11505)
  fixed annotations error on export (mne-tools#11435)
  DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506)
  BUG: updates for MPL 3.7 compatibility (mne-tools#11409)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner left review comments

@agramfort agramfort agramfort approved these changes

+1 more reviewer

@vpeterson vpeterson vpeterson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tsbinns @larsoner @agramfort @vpeterson @drammock