Changes to fields and description of NXdisk_chopper

[matthew-d-jones]

Since the existing NXdisk_chopper definitions appeared not to be fit for purpose I have been bold in making breaking changes. edit: changes are now non-breaking

Changes follow discussion of requirements for recording chopper data at the ESS.

I think the delay description requires further refinement.

[matthew-d-jones]

@zjttoefs I've created this as a draft PR for you to take a look at first. The phase description still makes little sense to me.

[zjttoefs]

Is there a need to make the changes breaking?

I don't think we need to remove anything. And the phase in time might better be called delay. No?

[matthew-d-jones]

The other breaking changes were removing the type and wavelength_range fields which don't make sense for a single disk, and changing the meaning of the radius field which made little sense to me.

[matthew-d-jones]

Just realised I carefully used American spelling everywhere to be consistent with "disk" in "NXdisk_chopper" but then used the term "anticlockwise".

[prjemian]

Re wavelength_range, isn't there always a range of wavelengths that depends on the chopper opening size and the rotation speed?

[matthew-d-jones]

Re wavelength_range, isn't there always a range of wavelengths that depends on the chopper opening size and the rotation speed?

I suppose there is technically a low bound on the energy due to the thickness of the disk, but otherwise not really with a single disk.

[zjttoefs]

I don't like the breaking changes being mixed in with the very valid additions and improvements proposed. (That includes the repurposing of slit_angle and phase.)

This can be easily done in two separate stages.

[mkoennecke]

There is a contradiction here: in the top documentation it says that there ought to be a NXdisk_chopper group per disk. Then the pair_separation does not make any sense as it implies a chopper pair.

Regarding the phase. Given there is a distance between two choppers. Then the second chopper has to open a little later then the first one in order to account for the flight time between the two choppers. This is expressed as an angular offset, the phase, in the chopper systems I had to deal with so far. This is something we read from the chopper controller, thus I want to have this field in order to record it. I do not mind the additional fields, they serve to describe the chopper in a more accurate way.

[matthew-d-jones]

@mkoennecke
There is already an angular phase field.

I originally removed the fields which don't make sense for a single disk, but Tobias is keen not to make breaking changes at the same time as the additions.
Given that there is nowhere else to store potentially useful information like wavelength_range and pair_separation, perhaps it is not unreasonable to store them in one, or both, of the two separate NXdisk_choppers making up the pair? Similarly, specifying type of "synchro_pair" could imply that this disk is one of those in the pair. We would need to change the wording to make this clear.

[zjttoefs]

I'm happy with this now.
@mkoennecke 's comments are also addressed, I would say: angular phase always existed and while the pair separation does contradict the premise of the base class, that is legacy and should be addresses separately.

[prjemian]

Ready to merge?

[zjttoefs]

I have given my approval. If no one else comments with requests for changes in the next days, I would merge.

[prjemian]

Ok, then. If no additional comments by May 22, then I will merge this.

[mkoennecke]

I am mostly OK with this. But I have a minor point: There is this slits field which indicates that a chopper may have more than one slit (They usually do). Should then slit_angle and slit_edges not be arrays of length nSlits? This might apply to the radius etc. fields too.

[matthew-d-jones]

I am mostly OK with this. But I have a minor point: There is this slits field which indicates that a chopper may have more than one slit (They usually do). Should then slit_angle and slit_edges not be arrays of length nSlits? This might apply to the radius etc. fields too.

slit_edges is already described as an array of length 2*nSlits:
https://github.com/nexusformat/definitions/pull/649/files#diff-965d206bd4d315d80b5f08de5a97d475R79

but I forgot to add the array description for slit_angle and slit_height, thanks for catching that.

[mkoennecke]

I am happy now.

[prjemian]

Ok by me

[zjttoefs]

I don't like the new change. It changes existing fields and hence breaks existing files.

The height is rarely different for different cut outs, I believe. And changing the slit angle to an array makes the slit_edges redundant.

[prjemian]

Seems this is not yet ready to merge.

[zjttoefs]

Since @mkoennecke framed his suggestions yesterday as "minor comments" I would think we're okay to merge after reverting the last commit and addressing the documentation improvement that @prjemian has highlighted.

[zjttoefs]

Two small changes and I'm happy (again ;-).

[zjttoefs]

I would make this "facing away from the source". Removes confusion if you ever put a chopper after the sample.

[matthew-d-jones]

👍 done

[zjttoefs]

Since we have three green ticks from reviewers, I would almost feel comfortable to merge. Especially since there are no changes (let alone breaking ones) to existing definitions.
But I'll give it a day or so, if @prjemian or @mkoennecke want to reassess the PR.

[prjemian]

change the each to each and I'm good overall

[zjttoefs]

To cut this short, while I can't edit @matthew-d-jones source for the PR, I'll merge and drop the "the" in master.

[zjttoefs]

Brilliantly referenced the wrong pull request, but the deed is done in 935afe4