imr-framework/pypulseq

Introduction of custom variables

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 🙄