Conversation
| "Note that a chopper rotating anti-clockwise is given a positive frequency,\n", | ||
| "while a chopper rotating clockwise is given a negative frequency.\n", |
There was a problem hiding this comment.
Why is this convention used?
There was a problem hiding this comment.
This is the convention from Nexus, but I don't like it either.
McStas also uses the sign of the frequency, and theirs is the opposite (positive for clockwise).
But regarding your comment below, yes I think I prefer using positive values for frequency and then a direction parameter.
| " tof.Chopper(\n", | ||
| " frequency=7 * Hz,\n", | ||
| " frequency=-7 * Hz,\n", |
There was a problem hiding this comment.
I wonder if relying on a sign convention will lead to too many bugs? Have you considered splitting this into frequency (positive) and direction: Literal['clockwise', 'anticlockwise']?
| # If the chopper is rotating anti-clockwise, we mirror the openings because the | ||
| # first cutout will be the last to open. | ||
| if self.frequency.value > 0: | ||
| open_times, close_times = ( | ||
| sc.array( | ||
| dims=close_times.dims, | ||
| values=(two_pi - close_times).values[::-1], | ||
| unit=close_times.unit, | ||
| ), | ||
| sc.array( | ||
| dims=open_times.dims, | ||
| values=(two_pi - open_times).values[::-1], | ||
| unit=open_times.unit, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The interface and need to handle this here feels very error-prone. Would it be possible to always specify openings in the order they appear, with positive angles? Any code for converting from different convention could be moved into helpers, such as Angles.from_nexus(**nexus_chopper_angles).
There was a problem hiding this comment.
Nice suggestion, but wouldn't this mean that the direction parameter would then be obsolete? Because we would always have the first opening crossing the beam first, regardless of the direction of rotation?
So I am wondering if it could me more error-prone in some cases?
There was a problem hiding this comment.
Not sure how different conventions for different things (rotation, phase, angles) interact, so this was just an idea. I don't mind either solution, but I thought that direction alone might not be sufficient, because there may be other conventions as well?
There was a problem hiding this comment.
Hmm, now that I think a bit more about this, I am wondering if we should do what you suggest, and not have a sign on the frequency, nor a direction parameter...
In the case of the dream choppers, there are two counter-rotating choppers, where one is above the beam and the other one is below. There is even a third chopper which is on the side:
- NeXus defines a TDC on the chopper, and an offset between the TDC and the position of the beam.
- McStas deals with this by defining the centre of the beam as the zero angle, meaning that the chopper above the beam is rotated 180deg compared to the one below.
I think defining things in terms of "the first window that is listed is the first one that will pass in front of the beam" makes sense. If a user has angles from a chopper diagram, and then they want to place that chopper on the side of the beam, they just have to either add 90deg to all their angles, or add 90deg to their phase.
To define the opening angles for a counter-rotating chopper, you have to reverse the order from what is in the NeXus.
I like the idea of having converters in separate functions.
There was a problem hiding this comment.
Does this mean we have to change nothing to the code but instead update the documentation to explain how to make a counter-rotating chopper?
There was a problem hiding this comment.
Does this mean we have to change nothing to the code but instead update the documentation to explain how to make a counter-rotating chopper?
I think we probably still need a direction parameter. Users typically have a diagram giving the opening angles:
It will not always be from_nexus or from_mcstas, sometimes you just want to check the values given in a paper. I think it will be useful to be able to set direction='anticlockwise'.
There was a problem hiding this comment.
About Angles.from_nexus(**nexus_chopper_angles), wouldn't we try and load everything from NeXus, not just the angles but the frequency, the phase etc?
For McStas, we cannot parse the instrument file, so I am not sure what to do there.
I am asking because I thought about either having Angles.counter_rotating(**angles), or a method on the chopper to flip the rotation, e.g.
chopper = Chopper(...)
chopper.counter_rotate() # would modify in-place the open and close anglesThe problem with the latter is that you don't necessarily remember how many times you called counter_rotate() and if you are doing stuff in the notebook, you may accidentally flip your chopper again by executing a cell once too many, so it would probably be better to have is set when constructing the chopper.
How about using classmethods?
Chopper.anti_clockwise(...)Chopper.from_nexus(...)
There was a problem hiding this comment.
👍 on the classmethods, this is more or less what I had in mind.
Yes, the idea was to not change this code here, but instead separate those concerns into independent helpers.
|
Implemented in #47 instead |
2 participants
Fix #5
Note that the definition of frequency has changed slightly:
This implies that all the frequencies in the tests and docs have now been converted to negative frequencies.
This also means that all users have to change the sign of their frequencies :-(