Add dream.load_nexus
#10
Conversation
I didn't follow the discussion. Can we rely on them being sorted from now on? |
We might, but we are not: Scipp will dynamically choose a faster or slower grouping algorithms, depending on whether they are sorted or not. There is no risk for correctness due to assumptions. |
| @pytest.mark.filterwarnings("ignore:Failed to load /entry/instrument/monitor_bunker") | ||
| @pytest.mark.filterwarnings("ignore:Failed to load /entry/instrument/monitor_cave") | ||
| @pytest.mark.filterwarnings("ignore:Failed to load /entry/instrument/polarizer/rate") | ||
| @pytest.mark.filterwarnings("ignore:Failed to load /entry/instrument/sans_detector") |
There was a problem hiding this comment.
Are you planning to remove those filters when we have a complete file?
There was a problem hiding this comment.
I suppose, what are you asking?
There was a problem hiding this comment.
I wanted to know if these are permanent and a problem with the loader or just temporary until we get better files.
| 'high_resolution', | ||
| 'sans', | ||
| ): | ||
| assert f'{name}_detector' in instr |
There was a problem hiding this comment.
In the GEANT4 loader, those are called name, i.e., without "_detector". We should use the same names in both and I'm in favour of the shorter names in the GEANT4 loader. What do you think?
There was a problem hiding this comment.
@celinedurniak Should we remove the suffix? The name is also repeated on the event data, e.g.,
/entry/instrument/endcap_forward_detector/endcap_forward_event_data
What do you prefer? Something like /entry/instrument/endcap_forward/events? Or something longer? Should we keep the naming from the NeXus file?
| See https://confluence.esss.lu.se/pages/viewpage.action?pageId=462000005 | ||
| and the ICD DREAM interface specification for details. |
There was a problem hiding this comment.
This isn't great. Are there publicly accessible resources about this? Can we make these public?
There was a problem hiding this comment.
Why public? This is an ESS repo, so I think everyone who needs access can get to the info?
There was a problem hiding this comment.
I don't know how that will work for visitors. But fine.
There was a problem hiding this comment.
I think this will only matter for instrument scientist, mainly during hot commissioning.
| - The high-resolution detector has a very odd numbering scheme. The SANS detector | ||
| is using the same, but is not populated in the current files. The scheme | ||
| attempts to follows some sort of physical ordering in space (x,y,z), but it | ||
| looks partially messed up. |
There was a problem hiding this comment.
Should these be issues to help us follow up on them?
There was a problem hiding this comment.
I am not sure. It depends on how we will handle versioning in the loader. Will we have to keep it working with the current files, if ECDC makes fixes/changes in the future?
There was a problem hiding this comment.
Maybe not the current ones as they should be replaced by newer ones, I guess. But ultimately, we will have to keep supporting old ones.
| 'mounting_unit': 5, | ||
| 'cassette': 6, | ||
| 'counter': 2, | ||
| 'strip': 256, |
There was a problem hiding this comment.
These names correspond to the ICD, not GEANT4 names on the confluence page. Are these the ones we want to go with? Should we rename the dims in the GEANT4 loader?
There was a problem hiding this comment.
I don't think we can necessarily keep the GEANT4 naming, since the NeXus files order things in a different way so logic folding according to GEANT4 names is not possible (in at least 1 case, I think).
src/ess/dream/io.py
Outdated
| dg = f[entry][()] | ||
| dg = snx.compute_positions(dg) | ||
| if not fold_detectors: | ||
| return dg |
There was a problem hiding this comment.
For readability, can you turn the everything below this line into a separate function and call it here?
SimonHeybrock
force-pushed
the
load-dream
branch
from
bd2bea1 to
0e955a8
Compare
@celinedurniak this replaces scipp/ess#199. I have adapted it for the latest file I have (which has sorted detector numbers).