Add support for counter rotating choppers#47
Conversation
| The phase of the chopper. The phase offset is applied in the opposite direction | ||
| to the chopper rotation direction. |
There was a problem hiding this comment.
Can you clarify this a bit more? Do I read this correctly that the meaning of phase changes when we change the rotation direction?
| phases = sc.arange(uuid.uuid4().hex, -1, nrot) * two_pi + self.phase.to( | ||
| unit='rad' | ||
| ) | ||
| ) * (int(self.direction == 'clockwise') * 2 - 1) |
There was a problem hiding this comment.
Like this, any typos in direction will mean "anticlockwise". How about either validating the direction, or using a dict:
| ) * (int(self.direction == 'clockwise') * 2 - 1) | |
| ) * {'clockwise': 1, 'anticlockwise': -1}[self.direction] |
There was a problem hiding this comment.
Yeah it's partly because I wasn't sure about the naming. I don't know if anticlockwise is more intuitive than counter-clockwise.
Also, is it anticlockwise or anti-clockwise?
The best would probably to use an enum, but I don't know if that makes it impractical to use?
chopper = tof.Chopper(
frequency=14*Hz,
direction=tof.AntiClockwise,
...
)Or maybe we should have two classes? ChopperClockwise and ChopperAnticlockwise?
A bit verbose but hard to get wrong?
There was a problem hiding this comment.
In the end, I used
Clockwise = Literal["clockwise"]
AntiClockwise = Literal["anticlockwise"]to define the two options. It seems to work fine, but I am not sure if there is anything bad/dangerous about doing it like that.
Co-authored-by: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com>
| distance: sc.Variable, | ||
| phase: sc.Variable, | ||
| direction: Literal['clockwise', 'anticlockwise'] = 'clockwise', | ||
| direction: Union[Clockwise, AntiClockwise] = Clockwise, |
There was a problem hiding this comment.
Very unusual, using a union of Literal...
There was a problem hiding this comment.
I know, I didn't even know that it would work.
Do you have a more common suggestion?
There was a problem hiding this comment.
Is using NewType better?
There was a problem hiding this comment.
Btw, using Literal was a suggestion by Copilot... ;-)
There was a problem hiding this comment.
Why don't you use a single literal, Literal['clockwise', 'anticlockwise']? Or an enum?
There was a problem hiding this comment.
Yes, I went with Enum in the end.
| phases = sc.arange(uuid.uuid4().hex, -1, nrot) * two_pi + self.phase.to( | ||
| unit='rad' | ||
| ) * (int(self.direction == 'clockwise') * 2 - 1) | ||
| ) * (int(self.direction is Clockwise) * 2 - 1) |
There was a problem hiding this comment.
So if I pass clokcwise it will still be interpreted as AntiClockwise?
2 participants
Add support for counter-rotating choppers via the
direction='anticlockwise'argument.After discussions in #44 , I decided that using a classmethod for this is not so useful because the computation of the times when the windows are open/closed are not done upon construction.
They depend on how many rotations need to be performed.
I think the easiest is to have the
directionargument, set toclockwiseby default so that current users do not have to change all their notebooks.We will most probably still use a
classmethodto create a chopper from nexus parameters.Supersedes #44
Fix #5