Skip to content

DOC: match_channel_orders works on Epochs and Evoked, too#12699

Merged
larsoner merged 10 commits intomne-tools:mainfrom
sappelhoff:doc_order
Jul 13, 2024
Merged

DOC: match_channel_orders works on Epochs and Evoked, too#12699
larsoner merged 10 commits intomne-tools:mainfrom
sappelhoff:doc_order

Conversation

@sappelhoff
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff commented Jul 4, 2024

minor doc contribution. All these classes: Raaw, Epochs, and Evoked work with the "match_channel_orders" function, as it just used the shared reorder_channels method under the hood.

This PR clarifies that.

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Jul 11, 2024

@larsoner do you think we could get this merged?

This is how it looks in rendered state: https://output.circle-artifacts.com/output/job/eb4af659-c944-4ed7-9743-737bc1e36f16/artifacts/0/html/generated/mne.match_channel_orders.html#mne.match_channel_orders

@cbrnr cbrnr added this to the 1.8 milestone Jul 11, 2024
@mscheltienne
Copy link
Copy Markdown
Member

mscheltienne commented Jul 11, 2024

Do we need a deprecation cycle for the argument name change?

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 11, 2024

I think it's very unlikely that users have passed this as a keyword argument. But at the same time it's pretty easy to tack on a *, raws=None at the end of the signature and if raws is not None: emit a warning and set insts=raws or whatever. @sappelhoff I'll let you decide if you think it's worth it or not, it's a coin toss to me :)

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Jul 11, 2024

Thanks for the note and the suggestions. I personally would go ahead without a raws=None parameter and without a deprecation cycle, as I agree that it's very unlikely to cause many breakages.

But at the same time it's pretty easy to tack on a *, raws=None at the end of the signature and if raws is not None: emit a warning and set insts=raws or whatever.

With this solution:

  1. would we keep documenting raws in the numpydoc docstring? If so, do we just informally write "this is deprecated, use 'insts' instead"?
  2. would we remove this "deprecation hack" after one or two releases? Or would we keep it?

I think if the answer to 2. is "remove it eventually", I would be more prone to adding this "safeguard" :-)

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 11, 2024

Would we keep documenting raws in the numpydoc docstring? If so, do we just informally write "this is deprecated, use 'insts' instead"?

Yes to both!

would we remove this "deprecation hack" after one or two releases? Or would we keep it?

We would remove it after a single release like we do with all deprecations currently

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Jul 11, 2024

ok, I'll add it in this case!

  1. not a lot of work
  2. may be safer for some
  3. not ugly in the long term

thanks

@sappelhoff sappelhoff requested a review from larsoner as a code owner July 12, 2024 08:38
@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Jul 12, 2024

the doc build failure seems unrelated:

E: Unable to locate package python3.12-venv

@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Jul 12, 2024

And I have no idea why this test is failing: https://github.com/mne-tools/mne-python/actions/runs/9906155680/job/27367213244?pr=12699#step:17:6028

the DeprecationWarning DOES seem to be raised, but isn't picked up somehow?

larsoner
larsoner reviewed Jul 12, 2024
View reviewed changes
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 12, 2024

Pushed a tiny commit to make the tests happy, marking for merge-when-green, thanks in advance @sappelhoff !

larsoner
larsoner reviewed Jul 12, 2024
View reviewed changes
@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Jul 13, 2024

Thanks for the fixes @larsoner :-)

@larsoner larsoner merged commit aa45f40 into mne-tools:main Jul 13, 2024
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jul 13, 2024

Thanks @sappelhoff !

@sappelhoff sappelhoff deleted the doc_order branch July 13, 2024 17:26
larsoner added a commit to scott-huberty/mne-python that referenced this pull request Jul 16, 2024
@larsoner
Merge remote-tracking branch 'upstream/main' into egi_annots_from_stim
b2e21aa 
* upstream/main: (252 commits)
  Disable the "Back to top" button in the documentation (mne-tools#12688)
  DOC: match_channel_orders works on Epochs and Evoked, too (mne-tools#12699)
  Scale points and labels in montage plot (mne-tools#12703)
  Add license header to mne.stats.erp (mne-tools#12712)
  Update license year to 2024 (mne-tools#12713)
  Add standardized measurement error (SME) (mne-tools#12707)
  ENH: Parallel example execution in doc build (mne-tools#12708)
  MAINT: Update PR template (mne-tools#12692)
  MAINT: Fix doc build (mne-tools#12706)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12702)
  Improve documentation of ylim argument through Evoked plotting function (mne-tools#12697)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12696)
  BUG: Fix bug with CSP rank="full" (mne-tools#12694)
  MRG: Add epochs metadata summary to HTML representation (mne-tools#12686)
  Correct `Epochs.apply_function` docstring (mne-tools#12691)
  FIX: Gracefully handle missing datetime in Eyelink File (mne-tools#12687)
  MAINT: Restore SciPy pre (mne-tools#12689)
  Enh single channel annotation (mne-tools#12669)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12682)
  Bump autofix-ci/action from 1.2 to 1.3 in the actions group (mne-tools#12681)
  ...
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

@drammock drammock Awaiting requested review from drammock drammock is a code owner

@cbrnr cbrnr Awaiting requested review from cbrnr

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@dengemann dengemann Awaiting requested review from dengemann

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.8

Development

Successfully merging this pull request may close these issues.

4 participants

@sappelhoff @mscheltienne @larsoner @cbrnr