Skip to content

Conversation

@SayanDey322
Copy link

Reference issue (if any)

What does this implement/fix?

Additional information

fit_params: dict | None = None,
max_iter: int | Literal["auto"] = "auto",
allow_ref_meg: bool = False,
verbose: Any = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

While Any would help this pass a type checker, IMO it is not a very helpful type annotation for end users, and more to the point, I don't think it is correct here? The Valid arguments for the Verbose parameter are actually constrained:

  • None,
  • True, False,
  • "INFO", "WARNING" "ERROR", "CRITICAL"
  • an integer like 0, 1, 2, etc.

Maybe we can use a TypeAlias? But AFAIK, we have not created any type aliases in MNE-Python yet, so I would like other devs to weigh in here first.

Copy link
Member

Choose a reason for hiding this comment

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

I think a TypeAlias would be good here and I would simplify to

VerboseT: TypeAlias = None | bool | Literal["debug", "info", "warning", "error"] | int

maybe we can do away with the upper-case variants for now

Copy link
Contributor

Choose a reason for hiding this comment

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

@larsoner that sounds good to me. Should we create a dedicated typing module ie. mne.utills.typing and define this there? Or is it too soon to create a dedicated module (given that this is the first TypeAlias).

Copy link
Member

Choose a reason for hiding this comment

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

I think mne.typing would make sense

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.

3 participants