Skip to content

Conversation

@nvaytet
Copy link
Member

@nvaytet nvaytet commented Jul 15, 2025

This is a necessary update after scipp/essreduce#253

Only the tests were updated, as the main mode of operation is to use a lookup table read from a file.

@nvaytet nvaytet requested a review from jl-wynen July 15, 2025 10:05
Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. Can you update the lower pin?

Also, did you check whether https://github.com/scipp/essdiffraction/blob/main/tools/dream-make-tof-lookup-table.ipynb still works?

@nvaytet
Copy link
Member Author

nvaytet commented Jul 15, 2025

Also, did you check whether https://github.com/scipp/essdiffraction/blob/main/tools/dream-make-tof-lookup-table.ipynb still works?

No, and that is an issue I was wondering about... how do we ensure these notebooks don't get out of date?
They don't really belong in the docs, but we should try and run them in the CI somehow?
Or we put them in the developer section of the docs?

Edit: I now moved the notebook inside the docs, and use a smaller number of neutrons.
I don't know if we want to clean up the file it writes to disk? Or comment that cell out?

" choppers=disk_choppers, neutrons=5_000_000, source_position=sc.vector([0, 0, 0], unit='m'),\n",
")\n",
"\n",
"wf[time_of_flight.NumberOfSimulatedNeutrons] = 200_000 # Increase this number for more reliable results\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add to the docs (here or in data.py) the number used for the table? Or is it encoded in the hdf5 file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't. We should probably make it a coordinate, like the other metadata?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it should be unaligned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that said, it's something specific to Tof. It's actually the neutrons that were in the source, not the neutrons that make it to the detectors.

If the simulation is done with mcstas, with parameter should not be there.
The other parameters (distamce and time resolution, etc.) belong with the table.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. But it can still be good to encode this kind of metadata in the file. But that is a longer discussion as we know...

For now, just add it to the docs so we can reproduce it later.

@jl-wynen jl-wynen mentioned this pull request Aug 4, 2025
@nvaytet nvaytet merged commit 621d197 into main Aug 5, 2025
4 checks passed
@nvaytet nvaytet deleted the new-tof-lut-workflow branch August 5, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants