Introduction of custom variables
schuenke opened this issue · 4 comments
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.
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?
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}")
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
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 🙄