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

Introduction of custom variables #199

Open
schuenke opened this issue Aug 29, 2024 · 4 comments
Open

Introduction of custom variables #199

schuenke opened this issue Aug 29, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@schuenke
Copy link
Collaborator

For PR #198, I was thinking of adding a types.py to define a custom variable for (gradient) channels, but I decided to create an Issue first and hear about your opinions.

For (grad) channels, the custom var could simply look like this:

from typing import Literal

# Define custom type for gradient channels
Channel = Literal['x', 'y', 'z']

And of course there would be more useful custom vars, for example for labels ('SLC', 'SEG', 'REP' etc).

The advantages would be:

  • improved readability ('Channel' or 'Direction' is much more descriptive than just 'str')
  • easier maintainability: if a new label is added, we only have to change it in one instead of XX positions/files
  • ensure consistent usage across the whole package
  • type safety and error prevention: type checker / linting will directly complain when using wrong variable values

Let me know what you think about this.

@schuenke schuenke added the enhancement New feature or request label Aug 29, 2024
@FrankZijlstra
Copy link
Collaborator

FrankZijlstra commented Aug 29, 2024

Is there a way to iterate over the values in a Literal? I think when defining something like this globally, it would be nice to then have a way to say:
for c in Channel, or to use len(Channel)

With that in mind, wouldn't an Enum be a better type, in that it allows both those things? The only issue I see there is that all old code becomes incompatible because passing 'x' needs to be replaced with Channel.X...

Edit: Python 3.11 has StrEnum which seems to allow also using string values?

@schuenke
Copy link
Collaborator Author

You can easily get what you asked for using

import typing

Channel = typing.Literal["x", "y", "z"]

VALID_CHANNELS = typing.get_args(Channel)
print(f"Valid channels are: {VALID_CHANNELS}")

@FrankZijlstra
Copy link
Collaborator

I did a quick scan of the codebase, found the following literals that could be typed like this:

  • Gradient channels
  • Labels
  • RF use
  • Grad units (Hz/m etc)
  • Slew units (Hz/m/s etc)
  • Trigger channel

@schuenke
Copy link
Collaborator Author

schuenke commented Sep 5, 2024

Perfect. Lets leave this open and expand the list if we realize it's not complete yet.
But maybe we can switch from the current flat-layout to the src-layout before introduction of the Literals to avoid problems like the one in #202. If we do both in parallel, merging will be a pain 🙄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants