-
Notifications
You must be signed in to change notification settings - Fork 2
Basic dream workflow #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/ess/dream/beamline.py
Outdated
| def flatten_detector_dimensions( | ||
| data: RawDetectorData[RunType], | ||
| ) -> FlatDetectorData[RunType]: | ||
| """Flatten logical detector dimensions to a single ``spectrum``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't RawDetectorData (from ESSreduce) flat already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's shaped like the logical dims. At least this is what the GEANT4 loader produces. Would you rather we load a flat array and have a separate step for folding afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is why you flatten it. We want to fold the flat version we get from ESSreduce, that is what also ESSsans will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed flattening
src/ess/dream/beamline.py
Outdated
| Flattened detector data with a ``spectrum`` dimension. | ||
| """ | ||
| logical_dims = {'module', 'segment', 'counter', 'wire', 'strip', 'sector'} | ||
| actual_dims = tuple(dim for dim in data.dims if dim in logical_dims) | ||
| return FlatDetectorData[RunType](data.flatten(dims=actual_dims, to='spectrum')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we even want to flatten?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some providers didn't work. In particular, the focussing step needs a spectrum dim. Should I make that instrument-specific so that it can use the logical dimensions?
Also, isn't there a massive performance penalty when binning in an additional dim like tof?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The steps should be fixed to work with any dims. If this is too much work, we should look into it.
src/ess/powder/conversion.py
Outdated
| def _to_dspacing_impl( | ||
| data: sc.DataArray, base_graph: dict[str, Callable[..., Any]] | ||
| ) -> sc.DataArray: | ||
| out = _restore_tof_if_in_wavelength(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to contradict the "Attention" section in the docstring above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. _restore_tof_if_in_wavelength is a bit funky, but it removes the wavelength and makes tof aligned. The latter part is no longer needed, this code is a leftover from when we used attrs. So I guess we can just delete the wavelength coord instead.
src/ess/powder/conversion.py
Outdated
|
|
||
| out = out.transform_coords('dspacing', graph=graph, keep_intermediate=False) | ||
| out = data.transform_coords('dspacing', graph=graph, keep_intermediate=False) | ||
| out.coords.pop('_tag_positions_consumed', None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you ensure with this mechanism? That a conversion actually happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ensures that positions in the output are unaligned. The conversion with calibration doesn't use positions directly but conceptually, d is derived from positions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this in a comment please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jl-wynen Has this been addressed somewhere? Can't see in the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done now. I actually simplified the conversion with positions because it doesn't need this mechanism.
src/ess/powder/types.py
Outdated
| class FlatDetectorData(sciline.Scope[RunType, sc.DataArray], sc.DataArray): | ||
| """Data (events / histogram) of a RawDetector flattened to ``detector_number``.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, not sure why this is needed.
a0a3466 to
1acddf9
Compare
1acddf9 to
2d97220
Compare
src/ess/dream/beamline.py
Outdated
| base = ('module', 'segment', 'counter', 'wire', 'strip') | ||
| if detector_name == DetectorName.high_resolution: | ||
| return DetectorDimensions(base + ('sector',)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you double check if this is actually correct? I remember that at least one detector was not prepresentable with these base dims.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not encountered this. But I only have simulated data. Do you have a NeXus file where it doesn't work? If so, we need to update the csv loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endcap detectors have a irregular structure that cannot be fully folded.
This is not a problem but note again that the counter may be reversed. It is
currently not clear if this is a bug in the file.
What do I do with this information? Do I just not fold the endcaps? Or do I only fold part of them, which part?
Or Do we just wait because it seems to be unclear whether real files will be like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it is unclear. Real file are (and will be) like this. It is impossible to (fully) fold. I guess we should do what is done in the linked file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing geant4 and ICD names and sizes (https://confluence.esss.lu.se/pages/viewpage.action?pageId=462000005), I don't think we can really represent both simulated and measured data in the same way. Some names are different as well as some sizes. And out test data doesn't even conform to the specs on the confluence page (e.g., the mantle has 7 modules where the pages says there should be 5).
So I would say we just live with the discrepancy. I can change the provider you commented on to check which dims exist in the data and return either the current set or the dims produced by the nexus loader. Got any better idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with any discrepancy, provided that this code is ensured to be Geant-4 specific.
| """Path to a GEANT4 CSV file for a diamond sample. | ||
| **Sample**: | ||
| - Diamond powder | ||
| - radius=0.0059 m | ||
| - yheight=0.05-1e-9 m | ||
| - material | ||
| reflections of powder from "C_Diamond.laz" (McStas file) | ||
| incoherent process with sigma=0.001 barns, packing_factor=1, | ||
| unit cell volume=45.39 angstrom^3 | ||
| absorption of 0.062 1/m | ||
| **Container**: | ||
| - radius=0.006 m | ||
| - yheight=0.05 m | ||
| - material | ||
| reflections of powder from "V.laz" (McStas file) | ||
| incoherent process with sigma=4.935 barns, packing factor=1, | ||
| unit cell volume=27.66 angstrom^3 | ||
| absorption of 36.73 1/m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to include this info in the docstrings, this makes the data module much more useful.
src/ess/dream/beamline.py
Outdated
| 'counter', | ||
| 'strip', | ||
| 'sector', | ||
| 'sumo_cass_cst', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? See
essdiffraction/src/ess/dream/io/nexus.py
Line 99 in 0a375c1
| 'sumo_cass_ctr': -1, # sumo*cassette*counter, irregular, cannot fold |
5151940 to
a5415e1
Compare
|
Any more comments? |
Fixes #35
This obviously needs a lot of work. But it is meant as a skeleton to add the rest of the milestone.
The data is currently private. I'm waiting for an ok from the instrument team before publishing it.