Conversation
|
Questions that remain answering
|
jl-wynen
left a comment
jl-wynen
left a comment
There was a problem hiding this comment.
I don not see any pixel binning here, only binning by position. Is the detector aligned with the instrument coord system? And does the mcstas file not contain pixel ids?
| sc.midpoints(da.coords['y']), | ||
| sc.midpoints(-da.coords['x'] if bank == 1 else da.coords['x']), | ||
| ) | ||
| ).transpose(da.dims) |
There was a problem hiding this comment.
Can you add a comment explaining what is going on here?
Why are you using 'x' as the z component of the vector? (You should drop the 'x' coord to avoid confusions down the line.)
There was a problem hiding this comment.
x is a detector local coordinate, after the detectors are rotated it points in the global z direction.
But this is changed. Now the rotation is applied explicitly.
src/ess/beer/io.py
Outdated
| da.coords['Ltotal'] = L1 + L2 | ||
| da.coords['two_theta'] = sc.acos( | ||
| (-da.coords['x'] if bank == 1 else da.coords['x']) / L2 | ||
| (da.coords['position'] - da.coords['sample_position']).fields.z / L2 |
There was a problem hiding this comment.
This doesn't look right because it doesn't account for the source position. Why don't you us tuse the predefined coord transform graph?
There was a problem hiding this comment.
(Implicit assumption in the old code was that incident beam is along z.)
The code now explicitly defines the incident beam direction, and uses the coordinate transformation graphs.
It does contain pixel ids, but the positions they are associated with in the detector component are wrong :/ I did try quite a bit to get binning by pixel id to work, but in the end the conclusion I reached is that the McStas "Nexus" files are not good enough to be amenable to automatic processing. There are just too many problems and missing information in the files. A major issue is that they don't use the NeXus classes correctly, so it's impossible to automate finding the correct component in the component list... Finding components by name break because the names of components can change between different versions of the model... So the less we use instrument component data the better. |
src/ess/beer/io.py
Outdated
| else sc.spatial.rotation( | ||
| value=[ | ||
| 0.0, | ||
| np.sin(-detector_rotation_angle / 2), |
There was a problem hiding this comment.
Can you move the - into the definition of the angle to make this more readable? It took me a moment to see the difference between the banks.
| '''Load beer McStas data from a file to a | ||
| data group with one data array for each bank. | ||
| ''' | ||
| if isinstance(f, str | Path): |
There was a problem hiding this comment.
You could use ess.reduce.nexus.open_nexus_file here. It handles both paths and existing file objects. And it bypasses locking on read only filesystems. But it uses ScippNexus objects, not h5py. So you would have to get the h5.Group out of that first. Your call.
There was a problem hiding this comment.
Good to know. But given how nexus-incompatible these files are, I suspect scippnexus might be confused by that, and that in the end we don't gain much by using it here, for now I'll just keep it as it is. We won't need to read McStas simulation files from a read only filesystem anyway.
There was a problem hiding this comment.
Opening a file with scippneux doesn't really do anything on top of what h5py does. But fair point
2 participants
Addresses part of #217 by binning the McStas data by "pixel" after loading and adapting the workflow to handle the pixel-binned data structure.