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

Do not show Annotated types in configuration options documentation #911

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Mar 29, 2024

I want to get rid of the Annotated bits e.g. here:
Screenshot 2024-03-29 at 14 28 15

and only include the "actual" type (the Literal[...] in this specific case).

Before merging …

  • Changelog has been updated (docs/source/changes.md)

@hoechenberger hoechenberger changed the title Do not show Annotated types in configuration options documentation [circle ds000248] Do not show Annotated types in configuration options documentation Mar 29, 2024
@larsoner
Copy link
Member

We will lose some information here like the Len(1, 4) or Ge etc. but I guess that's okay

@hoechenberger
Copy link
Member Author

hoechenberger commented Mar 29, 2024

We will lose some information here like the Len(1, 4) or Ge etc. but I guess that's okay

Yes but I don't think this stuff is very user-readable anyway, so I'd love to keep it out of this part of the docs. If we think we need to document it, we should add it to the docstring in "proper" words.

Right now it's an implementation detail to help us with validation mostly, no?

WDYT?

@larsoner
Copy link
Member

Okay by me. Some of it is obvious anyway like people shouldn't pass an empty list for ch_types etc.

@hoechenberger
Copy link
Member Author

hoechenberger commented Mar 29, 2024

https://output.circle-artifacts.com/output/job/71b0ad3a-e6cd-4551-a09f-e685b3995872/artifacts/0/site/settings/general.html#mne_bids_pipeline._config.ch_types

It's working

… and makes me wonder: huh, the default here actually doesn't match the annotation – it's an empty list, which is in violation of the annotation, which demands 1–4 elements:

ch_types: Annotated[Sequence[Literal["meg", "mag", "grad", "eeg"]], Len(1, 4)] = []

@larsoner Do you think we could change the default to None and simply use all available & supported channel types in this case? Otherwise, I don't know what a good default would look like.

@hoechenberger hoechenberger marked this pull request as ready for review March 29, 2024 15:06
@larsoner
Copy link
Member

Do you think we could change the default to None and simply use all available & supported channel types in this case? Otherwise, I don't know what a good default would look like.

Sure we could add this as an enhancement. At the moment it's not too bad since if users don't specify this they immediately get a clear error message saying they need to specify it (here I've removed ch_types from the ds004229 config):

$ pytest mne_bids_pipeline/ -k ds004229
...
E   ValueError: 1 validation error for user configuration from /home/larsoner/python/mne-bids-pipeline/mne_bids_pipeline/tests/configs/config_ds004229.py
E   ch_types
E     Value should have at least 1 item after validation, not 0 [type=too_short, input_value=[], input_type=list]
E       For further information visit https://errors.pydantic.dev/2.6/v/too_short

but ch_types = None meaning "autodetect and use all" seems like nicer behavior

@larsoner larsoner merged commit 9706946 into mne-tools:main Mar 29, 2024
52 checks passed
@larsoner
Copy link
Member

Thanks @hoechenberger

@larsoner
Copy link
Member

@hoechenberger
Copy link
Member Author

Argh…

I really totally lost track of how all that stuff works, I find our CircleCI config extremely confusing :(
Will try to take a look at it later

larsoner added a commit to larsoner/mne-bids-pipeline that referenced this pull request Apr 16, 2024
* upstream/main:
  change default for info to use for inverse mne-tools#905 (mne-tools#919)
  Improve documentation and config validation of `loose` and `depth` parameters; drop support for `loose=None` (mne-tools#915)
  enhance documentation of caching, continuation of mne-tools#914 (mne-tools#918)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#917)
  Restructure configuration options documentation sections (mne-tools#914)
  Try to fix documentation deployment (mne-tools#913)
  Do not show `Annotated` types in configuration options documentation (mne-tools#911)
  Add number of subjects to grand-average report (cont'd) (mne-tools#910)
  MAINT: Ensure input changes cause output changes (mne-tools#904)
  Render type annotations in the documentation again (mne-tools#909)
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.

2 participants