Skip to content

WIP: TFR Doc/test changes #7998

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
larsoner merged 4 commits into from
Jul 13, 2020
Merged

WIP: TFR Doc/test changes #7998

larsoner merged 4 commits into from
Jul 13, 2020

Conversation

@bloyl
Copy link
Contributor

@bloyl bloyl commented Jul 10, 2020

Closes #7978

@sappelhoff @larsoner @agramfort

I have currently commented out the tests for the complex return type as they don't make sense and thus shouldn't pass.
I could remove them and just leave them as smoke tests or if anyone has a suggestion on what to regression test to I could do that. Just let me know what you think is best.

@drammock I'll admit that I don't know how all the doc linking stuff works so please let me know what needs to be changed.

drammock
drammock reviewed Jul 10, 2020
View reviewed changes
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

In addition to my suggested formatting edits, this seems like a good candidate for a docdict entry.

mne/time_frequency/tfr.py Outdated
Comment on lines 692 to 694
If False return an EpochsTFR containing separate tfrs for each epoch.
If True return an AverageTFR containing the average of all tfrs across
epochs.
Copy link
Member

@drammock drammock Jul 10, 2020

Choose a reason for hiding this comment

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

Suggested change
If False return an EpochsTFR containing separate tfrs for each epoch.
If True return an AverageTFR containing the average of all tfrs across
epochs.
If ``False`` return an `EpochsTFR` containing separate TFRs for each
epoch. If ``True`` return an `AverageTFR` containing the average of all
TFRs across epochs.

mne/time_frequency/tfr.py Outdated
Comment on lines 696 to 698
.. note:: Using `average=True` is functionally equivlent to using
`average=False` followed by `EpochsTFR.average()`, but will be more
memory efficient.
Copy link
Member

@drammock drammock Jul 10, 2020

Choose a reason for hiding this comment

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

Suggested change
.. note:: Using `average=True` is functionally equivlent to using
`average=False` followed by `EpochsTFR.average()`, but will be more
memory efficient.
.. note::
Using ``average=True`` is functionally equivalent to using
``average=False`` followed by ``EpochsTFR.average()``, but is
more memory efficient.

mne/time_frequency/tfr.py Outdated
Comment on lines 845 to 847
.. note:: Using `average=True` is functionally equivlent to using
`average=False` followed by `EpochsTFR.average()`, but will be more
memory efficient.
Copy link
Member

@drammock drammock Jul 10, 2020

Choose a reason for hiding this comment

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

Suggested change
.. note:: Using `average=True` is functionally equivlent to using
`average=False` followed by `EpochsTFR.average()`, but will be more
memory efficient.
.. note::
Using ``average=True`` is functionally equivalent to using
``average=False`` followed by ``EpochsTFR.average()``, but is
more memory efficient.

mne/time_frequency/tfr.py Outdated
Comment on lines 841 to 843
If False return an EpochsTFR containing separate tfrs for each epoch.
If True return an AverageTFR containing the average of all tfrs across
epochs.
Copy link
Member

@drammock drammock Jul 10, 2020

Choose a reason for hiding this comment

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

Suggested change
If False return an EpochsTFR containing separate tfrs for each epoch.
If True return an AverageTFR containing the average of all tfrs across
epochs.
If ``False`` return an `EpochsTFR` containing separate TFRs for each
epoch. If ``True`` return an `AverageTFR` containing the average of all
TFRs across epochs.

@sappelhoff
Copy link
Member

sappelhoff commented Jul 11, 2020

Thanks @bloyl the docstring is now much more understandable and clear.

re: test --> I'd either remove it or make it meaningful, but not comment it out and let it sit there.

@drammock I'll admit that I don't know how all the doc linking stuff works so please let me know what needs to be changed.

that works by using this file: https://github.com/mne-tools/mne-python/blob/master/mne/utils/docs.py

see also the explanation that I received some months ago: #7474 (comment)

larsoner
larsoner reviewed Jul 13, 2020
View reviewed changes
mne/time_frequency/tests/test_tfr.py
epochs_power_2 = abs(epochs_power_complex)
epochs_power_3 = epochs_power_2.copy()
epochs_power_3.data[:] = np.inf # test that it's actually copied
assert_array_almost_equal(epochs_power_2.data, epochs_power_picks.data)
Copy link
Member

@larsoner larsoner Jul 13, 2020

Choose a reason for hiding this comment

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

Instead of killing these, if they should check the opposite, you could do something like:

assert not np.allclose(..., atol=1-20)

Or whatever makes the point that they actually shouldn't be equal

agramfort
agramfort approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

maybe rebase on master to get more CIs green?

@larsoner larsoner merged commit 18ffbfa into mne-tools:master Jul 13, 2020
@larsoner
Copy link
Member

larsoner commented Jul 13, 2020

Thanks @bloyl

larsoner added a commit to larsoner/mne-python that referenced this pull request Jul 14, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into refactor
c1f79f7 
* upstream/master: (21 commits)
  MRG: Add SSP projectors to Report (mne-tools#7991)
  BUG: Fix warning (mne-tools#8006)
  WIP: TFR Doc/test changes (mne-tools#7998)
  MAINT: Remove numpydoc test on 3.6 (mne-tools#8005)
  MAINT: Better error message for mismatch (mne-tools#8007)
  MRG: Allow removal of active projectors if channels they applied to have meanwhile been dropped (mne-tools#8003)
  MRG Freesurfer coordinate frame tutorial (mne-tools#7578)
  FIX: Fix stockwell checks (mne-tools#7996)
  MRG, ENH: Add array-spacing plugin and reorganize deps (mne-tools#7997)
  MRG, ENH: Reduce memory usage of Welch PSD (mne-tools#7994)
  STY: One more [ci skip]
  STY: Docstyle [ci skip]
  Report parsing (mne-tools#7990)
  MRG, ENH: BrainVision impedance parsing (mne-tools#7974)
  BUG: Fix missing source space points (mne-tools#7988)
  [MRG] Strip base directory name from Report captions when using parse_folder (mne-tools#7986)
  DOC: Update estimates [skip travis] (mne-tools#7987)
  DOC: Try to improve find_bad_channels_maxwell doc (mne-tools#7982)
  VIZ, BUG: fix tickmarks in evoked topomap colorbar (mne-tools#7980)
  Add low-pass filter to find_bad_channels_maxwell() (mne-tools#7983)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drammock drammock drammock left review comments

@agramfort agramfort agramfort approved these changes

@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.

DOC: order of operation in TFR functions needs to be clarified

5 participants

@bloyl @sappelhoff @larsoner @agramfort @drammock