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

BUG: Fix bug with Report.add_ica component number #12156

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

larsoner
Copy link
Member

Closes mne-tools/mne-bids-pipeline#573

cc @hoechenberger

See for example https://mne.tools/mne-bids-pipeline/1.4/examples/ds000248_ica/sub-01_task-audiovisual_report.html#ICA

  1. Fix component numbers which used to just be range(len(picks)) as noted in In _proc-ica_report.html, the captions of the ICA properties plots are incorrect mne-bids-pipeline#573
  2. Always write the cumulative variance explained. Even though it's not informative for the first component, it makes the slider behavior between the first and second components more uniform and thus easier to use.
  3. Fix bug where {round(x)} was used, but round always returns a float so these were displayed with .0 at the end, which is misleading (looks like there is one decimal point of precision when there are none). This now formats better with {x:0.1f} which is more accurate in the same number of characters.

@hoechenberger
Copy link
Member

This looks good to me

And reminded me that we're still reporting an incorrect explained variance here

I think we fixed it in the pipeline ... but let's tackle this later

@hoechenberger
Copy link
Member

@larsoner I've found it, we want to use this:

#11141

@larsoner
Copy link
Member Author

@larsoner I've found it, we want to use this:

I looked into doing this briefly. I'm a bit worried that it will be expensive and slow. If you have 10 components for example you'll have to do ica.apply 10 times. If you have 2 channel types you now have to do it 20 times, etc. For now I'll remove the explained variance stuff because it's wrong -- it's using PCA explained variance not var exp by each ICA component.

And even once we do it per component, we probably don't want a cumulative one since they aren't necessarily orthogonal (I think?) you could end up with more than 100% variance explained if you add up the variances explained by all components. In that sense it doesn't really make sense to make it cumulative.

@larsoner
Copy link
Member Author

@hoechenberger if you think it's worth the computational cost feel free to open an issue pointing to this discussion so we don't forget to add it back in. We might want for example exp_var=True | False kwarg to say whether or not to do these computations

@hoechenberger
Copy link
Member

We might want for example exp_var=True | False kwarg to say whether or not to do these computations

Absolutely!

@larsoner
Copy link
Member Author

larsoner commented Nov 1, 2023

Failures are unrelated and should be fixed by fixes I snuck into #12026

@larsoner larsoner enabled auto-merge (squash) November 1, 2023 18:54
@larsoner larsoner merged commit 70a915b into mne-tools:main Nov 2, 2023
28 checks passed
@larsoner larsoner deleted the fix-component branch November 2, 2023 20:54
@hoechenberger
Copy link
Member

Thanks @larsoner!

hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Nov 3, 2023
Tries to fix CI error we get for mne-bids-pipeline:
mne-tools/mne-bids-pipeline#807 (comment)

This is a followup-fix for mne-tools#12156
larsoner added a commit to larsoner/mne-python that referenced this pull request Nov 3, 2023
* upstream/main: (35 commits)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
  Try to fix ICA Report (mne-tools#12167)
  BUG: Fix bug with Report.add_ica component number (mne-tools#12156)
  MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163)
  DOC: fix sphinx style typos (mne-tools#12161)
  MAINT: Fix linkcheck (mne-tools#12162)
  ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026)
  Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118)
  ENH: Collapse only in doc gen (mne-tools#12145)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12155)
  BUG: Fix bug with interior points not showing (mne-tools#12148)
  ENH: Warn about versions in sys_info (mne-tools#12146)
  Fix in conftest.py (mne-tools#12150)
  ENH: set color for bad channel with spatial_colors=True (mne-tools#12142)
  DOC: Better documentation of realign_raw (mne-tools#12135)
  Add mne-icalabel wildcard (mne-tools#12143)
  Remove LGTM.com configuration file (mne-tools#12139)
  DOC: Fix typo found by codespell (mne-tools#12140)
  DOC: Document governance updates (mne-tools#12133)
  ...
larsoner added a commit to JD-Zhu/mne-python that referenced this pull request Nov 3, 2023
* upstream/main: (26 commits)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
  Try to fix ICA Report (mne-tools#12167)
  BUG: Fix bug with Report.add_ica component number (mne-tools#12156)
  MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163)
  DOC: fix sphinx style typos (mne-tools#12161)
  MAINT: Fix linkcheck (mne-tools#12162)
  ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026)
  Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118)
  ENH: Collapse only in doc gen (mne-tools#12145)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12155)
  BUG: Fix bug with interior points not showing (mne-tools#12148)
  ENH: Warn about versions in sys_info (mne-tools#12146)
  Fix in conftest.py (mne-tools#12150)
  ENH: set color for bad channel with spatial_colors=True (mne-tools#12142)
  DOC: Better documentation of realign_raw (mne-tools#12135)
  Add mne-icalabel wildcard (mne-tools#12143)
  Remove LGTM.com configuration file (mne-tools#12139)
  ...
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 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

Successfully merging this pull request may close these issues.

In _proc-ica_report.html, the captions of the ICA properties plots are incorrect
3 participants