Add config option to disable HTML repr by cbrnr · Pull Request #11159 · mne-tools/mne-python

cbrnr · 2022-09-13T13:43:27Z

Open questions:

~~I added four lines of code at the beginning of _repr_html_() of each class that defines this method. We could outsource this (in a decorator?), but it's probably overkill right now.~~ I refactored this into a decorator, I think this is cleaner even for the few classes we have.
I didn't find a way to set a config option only temporarily (for the current session without writing to the config file) – is this possible somehow? set_config() doesn't seem to support it, but maybe I missed a context manager or something similar?

agramfort

could we test this so the new lines are covered? 🙏 @cbrnr

cbrnr · 2022-09-14T06:00:26Z

Sure, I'll add a test. Does anyone have an answer to my second question? Basically, I'm looking for something like mne.set_log_level(), which is only valid within the current session (so not permanently).

agramfort · 2022-09-14T09:20:44Z

doc/changes/latest.inc

 - Add example of how to obtain time-frequency decomposition using narrow bandpass Hilbert transforms to :ref:`ex-tfr-comparison` (:gh:`11116` by `Alex Rockhill`_)
 - Add ``==`` and ``!=`` comparison between `mne.Projection` objects (:gh:`11147` by `Mathieu Scheltienne`_)
 - Add ``encoding`` parameter to :func:`mne.io.read_raw_edf` and :func:`mne.io.read_raw_bdf` to support custom (non-UTF8) annotation channel encodings (:gh:`11154` by `Clemens Brunner`_)
+- Add config option ``MNE_REPR_HTML`` to disable HTML repr in notebook environments (:gh:`11159` by `Clemens Brunner`_)


looking at the doc and thinking where this information could be added I found

https://mne.tools/stable/auto_tutorials/intro/50_configure_mne.html

and this tells me that maybe using mne config mechanism would be the expect thing to do...

what do you think @cbrnr @drammock ?

I'm not sure I understand. Do you want to describe the new config option in the docs? We haven't done this for the other existing options though.

agramfort · 2022-09-14T10:15:56Z

what other options do you have in mind?

…

Message ID: ***@***.***>

cbrnr · 2022-09-14T11:49:27Z

If we haven't described our config options so far, I think we should describe all of them. But maybe @drammock knows if there's already some documentation.

larsoner · 2022-09-14T13:30:19Z

Does anyone have an answer to my second question? Basically, I'm looking for something like mne.set_log_level(), which is only valid within the current session (so not permanently).

The current way to get this behavior is just to set os.environ['...']. It will be temporary to that session

larsoner · 2022-09-14T13:32:39Z

... and I don't think there is a single place where we describe all options. Instead we try to describe each option around where it's used, e.g., the 3D options should be described in set_3d_options. For this new param since it's more "global" in scope like set_log_level, we could just add it to the config tutorial for now, and figure out in a separate issue if we want to add all options somewhere else later (I'm not yet convinced we should/need to so we should discuss separately).

cbrnr · 2022-09-14T14:01:57Z

The current way to get this behavior is just to set os.environ['...']. It will be temporary to that session

OK, that's what I did in the test.

Re describing config options, I'm fine with descriptions in different places, but maybe an overview of all possible values would be nice. Maybe even built into mne.get_config() somehow? Whatever we decide, I'd vote for tackling this in a separate PR, since this is a bit outside the scope of this PR.

drammock · 2022-09-14T14:26:54Z

I'm fine with descriptions in different places, but maybe an overview of all possible values would be nice. Maybe even built into mne.get_config() somehow?

mne.get_config("") returns a list of all valid config keys. (no explanation of what they do though, but fodder for a search engine or git grep).

cbrnr · 2022-09-14T16:13:59Z

mne.get_config("") returns a list of all valid config keys. (no explanation of what they do though, but fodder for a search engine or git grep).

That's probably good enough for now. WDYT @agramfort?

drammock · 2022-09-14T16:21:12Z

mne.get_config("") returns a list of all valid config keys. (no explanation of what they do though, but fodder for a search engine or git grep).

If we wanted a more thorough documentation of what they all do, a nice way to do it would be for mne.get_config("") to return a dict instead of a tuple, with the dict values being a short one-sentence description of what each key does and its valid values.

cbrnr · 2022-09-14T17:22:56Z

I could do that? In this PR?

drammock · 2022-09-14T17:37:42Z

I could do that? In this PR?

go for it. mne.get_config("") is mostly there for informational/interactive purposes, it's only used twice in the codebase in ways that should be compatible with dict too:

$ git grep "get_config('')"
mne/utils/tests/test_config.py:24:    assert (len(get_config('')) > 10)  # tuple of valid keys
tutorials/intro/50_configure_mne.py:84:assert 'MNEE_USE_CUUDAA' not in mne.get_config('')

drammock · 2022-09-14T17:39:41Z

oh, rereading, maybe you weren't volunteering :) and even if you were, it should probably be a separate PR.

cbrnr · 2022-09-14T17:59:16Z

That's OK, I'll do it in a separate PR. So I guess this one is ready?

drammock

LGTM, just one tiny suggestion. Thanks @cbrnr

mne/utils/misc.py

cbrnr · 2022-09-15T12:48:10Z

All green – @agramfort please feel free to merge if all of your comments have been addressed!

agramfort · 2022-09-15T12:56:35Z

thanks @cbrnr

can you open issues for the next PRs to complete to follow up? 🙏

larsoner added this to the 1.2 milestone Sep 13, 2022

agramfort reviewed Sep 13, 2022

View reviewed changes

agramfort reviewed Sep 14, 2022

View reviewed changes

drammock approved these changes Sep 14, 2022

View reviewed changes

mne/utils/misc.py Outdated Show resolved Hide resolved

cbrnr added 8 commits September 15, 2022 13:13

Add config option to disable HTML repr

7ad7d38

Add changelog entry

adf9913

Refactor into decorator

4b8fb0b

Fix style

ed9820a

Fix docstring style

c60bcb0

Add kwargs

efa69b6

Add test

feaab4c

Use lowercase config values

4810ac5

cbrnr force-pushed the repr-html-config branch from a6f24cb to 4810ac5 Compare September 15, 2022 11:15

agramfort approved these changes Sep 15, 2022

View reviewed changes

agramfort merged commit 63b430e into mne-tools:main Sep 15, 2022

cbrnr deleted the repr-html-config branch September 15, 2022 12:57

cbrnr mentioned this pull request Sep 15, 2022

Extend mne.get_config("") to return a dict #11164

Closed

Uh oh!

Conversation

cbrnr commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

cbrnr commented Sep 14, 2022

Uh oh!

agramfort Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

cbrnr Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

agramfort commented Sep 14, 2022 via email

Uh oh!

cbrnr commented Sep 14, 2022

Uh oh!

larsoner commented Sep 14, 2022

Uh oh!

larsoner commented Sep 14, 2022

Uh oh!

cbrnr commented Sep 14, 2022

Uh oh!

drammock commented Sep 14, 2022

Uh oh!

cbrnr commented Sep 14, 2022

Uh oh!

drammock commented Sep 14, 2022

Uh oh!

cbrnr commented Sep 14, 2022

Uh oh!

drammock commented Sep 14, 2022

Uh oh!

drammock commented Sep 14, 2022

Uh oh!

cbrnr commented Sep 14, 2022

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbrnr commented Sep 15, 2022

Uh oh!

agramfort commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbrnr commented Sep 13, 2022 •

edited

Loading