Skip to content

MRG: Issue warning when exporting with unapplied projectors and improve docstring#9994

Merged
larsoner merged 14 commits intomne-tools:mainfrom
mscheltienne:warning_export_unapplied_proj
Nov 12, 2021
Merged

MRG: Issue warning when exporting with unapplied projectors and improve docstring#9994
larsoner merged 14 commits intomne-tools:mainfrom
mscheltienne:warning_export_unapplied_proj

Conversation

@mscheltienne
Copy link
Copy Markdown
Member

@mscheltienne mscheltienne commented Nov 11, 2021

I added a warning when exporting with unapplied projectors to all 4 export functions:

  • mne.export.export_raw
  • mne.export.export_epochs
  • mne.export.export_evokeds
  • mne.export.export_evokeds_mff

I added this warning in their docstrings:

.. warning::
        Export does not apply projector. Unapplied projectors will be lost.
        Consider using :meth:`mne.io.Raw.apply_proj` before exporting.

Test for warning when exporting evoked with unapplied proj is missing because I didn't have any good idea on how to add a projector to the EGI evoked instance loaded.

@mscheltienne mscheltienne mentioned this pull request Nov 11, 2021
3 tasks
@mscheltienne
Copy link
Copy Markdown
Member Author

I don't know how to fix this style issue.. any idea is welcome.

Notes
-----
.. versionadded:: 0.24
%(export_warning_note_evoked)s
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if it's here or above, but:

E   mne.export._egimff.export_evokeds_mff : GL03 : Double line break found; please use only one blank line to separate sections or paragraphs, and do not leave blank lines at the end of docstrings

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, same as before this additional change... I'm sorry, but I don't get why this one is being raised. Is it because the docstring starts with a 'summary' line and does not have a 'description' before the warning?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the multi-line string export_warning_note starts and ends with a newline. Probably you need to remove one or both of those. I'd guess it's the trailing newline.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like that's not it.. or I missed something.

Notes
-----
.. versionadded:: 0.24
%(export_warning_note_evoked)s
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the multi-line string export_warning_note starts and ends with a newline. Probably you need to remove one or both of those. I'd guess it's the trailing newline.

@cbrnr cbrnr changed the title Issue warning when exporting with unapplied projectors and improve docstring. Issue warning when exporting with unapplied projectors and improve docstring Nov 12, 2021
@mscheltienne
Copy link
Copy Markdown
Member Author

@cbrnr Ahah, I didn't notice I put one in the title again!

@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 12, 2021

@drammock Let's see if this fixes it. I thought the fill_doc decorator was stripping before/after.

@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Nov 12, 2021

Ahah, I didn't notice I put one in the title again!

And in commit messages... 😄

@drammock
Copy link
Copy Markdown
Member

@mscheltienne here's how I found/fixed the problems:

in ipython REPL, our rendered docstrings are visible in plaintext via mne.Epochs.export? or mne.export.export_evokeds_mff?. That made it pretty clear to see, e.g., where the "two blank lines" was happening. The Epochs.export problem was more subtle, but the fix was based on the intuition that rST .. whatever:: blocks usually need a blank line above and/or below (but numpydoc requires it be at most one line)

@drammock
Copy link
Copy Markdown
Member

...of course even that was apparently not enough, because I forgot (for the hundredth time) that subsequent make html_dev-noplot calls will ignore unchanged files, so fixing the errors one at a time actually masked some errors from me on subsequent runs. Will push more fixes momentarily.

@larsoner
Copy link
Copy Markdown
Member

FWIW I usually find it easiest not to have a newline at the beginning or end in the replacement string. Then the newlines you use in the doc will be the only newlines (at the beginning and end) that you get, so it generally simplifies things

@mscheltienne
Copy link
Copy Markdown
Member Author

Thanks for the tip with IPython and ?, it makes it way easier to see the double line-skip indeed.

@mscheltienne mscheltienne changed the title Issue warning when exporting with unapplied projectors and improve docstring MRG: Issue warning when exporting with unapplied projectors and improve docstring Nov 12, 2021
@larsoner larsoner merged commit 6509aa8 into mne-tools:main Nov 12, 2021
@larsoner
Copy link
Copy Markdown
Member

Thanks @mscheltienne @drammock !

larsoner pushed a commit that referenced this pull request Nov 12, 2021
…ve docstring (#9994)

* Add a warning when exporting with unapplied projectors.

* Add test for raw and epochs.

* Fix for evoked.

* Fix typo in docstring + add additional warning for unapplied projs.

* Add entry to changelog [ci skip]

* add pr id.

* Fix style.

* Revert.

* Move common docstring to docdict. Add to Raw.export and Epochs.export. [skip azp] [skip actions]

* Move warnings to note section. [skip azp] [skip actions]

* attempt to fix style. [skip azp] [skip actions]

* attempt 2 [skip azp] [skip actions]

* fix whitespace [skip actions][skip azp]

* more whitespace fixes [skip actions][skip azp]

Co-authored-by: Daniel McCloy <dan@mccloy.info>
@mscheltienne mscheltienne deleted the warning_export_unapplied_proj branch November 12, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants