Skip to content

Basic workflow for Bragg peak monitor#69

Merged
jl-wynen merged 15 commits intomainfrom
bragg-peak-monitor-2
Oct 8, 2025
Merged

Basic workflow for Bragg peak monitor#69
jl-wynen merged 15 commits intomainfrom
bragg-peak-monitor-2

Conversation

@jl-wynen
Copy link
Copy Markdown
Member

@jl-wynen jl-wynen commented Oct 2, 2025

Part of #67. This adds a new single crystal diffraction workflow for BIFROST's Bragg peak monitor. I separated it from the main workflow because we want need to treat the monitor as a detector and handle it differently from the inelastic detectors.

The workflow will need some adjustments to make it properly load a Bragg peak monitor from nexus. But we can only really do that once we have a file.

I will add extra providers to compute the Q projections directly for live data in a separate PR. Please check that the current implementation makes sense!

@jl-wynen jl-wynen requested a review from g5t October 2, 2025 13:44
@jl-wynen jl-wynen marked this pull request as ready for review October 3, 2025 15:16
@jl-wynen jl-wynen requested a review from nvaytet October 6, 2025 07:24
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This notebook is missing from the docs index and thus not rendered in the built docs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Line-comments aren't possible on Notebooks?
It's still not in the NeXus files, so a change is needed anyway in the future and this can be ignored in favor of that future change:
We now know where the actual Bragg peak monitor is located: 800 mm from the sample, 57 degrees rotated from the tank centerline (with the same sense and rotation axis as a4). Changing the location slightly changes the range of |Q| but doesn't otherwise change the overall impression.

"\n",
"# Increase this number for more reliable results:\n",
"workflow[NumberOfSimulatedNeutrons] = 200_000"
"workflow[NumberOfSimulatedNeutrons] = 600_000"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you need to bump the number for building the docs? Did it not look right with 200_000?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I needed to bump it up beacause the increased range.
The old number actually lead to some gaps in the table even for the old range.

Comment thread src/ess/bifrost/single_crystal/q_map.py Outdated
import scipp as sc
from matplotlib.axes import Axes
from matplotlib.patches import Circle
from plopp.widgets import Box
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Depending on lazy_loader, this may now make ipywidgets a hard dependency of essspectroscopy. Not sure if we want that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Probably not. I moved the import into the function.

Comment thread src/ess/bifrost/single_crystal/q_map.py Outdated

def update_circle(q_range: tuple[float, float]) -> None:
lo_circle.radius, hi_circle.radius = q_range
q_map_fig.fig.canvas.draw_idle()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
q_map_fig.fig.canvas.draw_idle()
q_map_fig.canvas.draw()

This sfould also work

Comment thread src/ess/bifrost/single_crystal/q_map.py Outdated
lo_circle.radius, hi_circle.radius = q_range
q_map_fig.fig.canvas.draw_idle()

pp.View(pp.Node(update_circle, slider_node))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a comment as to why this View is needed?
And maybe a TODO to remove it once we release the changes with the leaf nodes?

@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented Oct 6, 2025

I implemented a coloured range under the ROI. This was a little tricky because of MPL requiring different data formats for creating the fill, updating it, and for the lines. I hope we can reuse this elsewhere.

Comment thread src/ess/bifrost/single_crystal/q_map.py Outdated
return self._r_outer

@property
def closed_xy(self) -> npt.NDArray[float]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

closed_xy and open_path -> standardize the names?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

)


def make_q_map(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not clear on whether the below would be needed for ESSlivedata? If so, is there anything that can be made plotting-lib independent?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How would that work? This code has to slice the data in the widget (plopp graph). So you would need to do that in the dashboard frontend. I didn't think this would work and so was going to implement something similar to #66

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is just that I did not understand whether such a complicated setup code would also be needed make a regular plot.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or is that what you meant with

I will add extra providers to compute the Q projections directly for live data in a separate PR. Please check that the current implementation makes sense!
?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. This setup is only complicated because the code draws the circles and annulus into the plot. Slicing itself is pretty much trivial.

jl-wynen and others added 2 commits October 7, 2025 09:38
Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
Copy link
Copy Markdown
Member

@nvaytet nvaytet left a comment

Choose a reason for hiding this comment

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

Looks good to me, but maybe we should also get the ok from @g5t ?

Copy link
Copy Markdown
Collaborator

@g5t g5t left a comment

Choose a reason for hiding this comment

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

I think a fixed a3 step size is more useful than a fixed number of a3 steps. Fixing the limits as well would work, and a user (or NICOS) should know the scan limits in advance.

events: sc.DataArray,
q_parallel_bins: int | sc.Variable,
q_perpendicular_bins: int | sc.Variable,
sample_rotation_bins: int | sc.Variable,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe a bit too particular:
when the range of data left in sliced after the |Q| slicing below changes, this constant-number-of-a3-bins approach causes the bin-widths to change. Instead I think a constant a3-bin-width with overall bin-limits set by the range in sliced would be better (or just an explicit bin axis, e.g., sc.linspace(dim='a3', start=0, stop=360, step=0.5, unit='deg'))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I changed it to a constant bin width. That should be enough for now. We can change to passing concrete bin edges later if need be.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Line-comments aren't possible on Notebooks?
It's still not in the NeXus files, so a change is needed anyway in the future and this can be ignored in favor of that future change:
We now know where the actual Bragg peak monitor is located: 800 mm from the sample, 57 degrees rotated from the tank centerline (with the same sense and rotation axis as a4). Changing the location slightly changes the range of |Q| but doesn't otherwise change the overall impression.

@jl-wynen jl-wynen enabled auto-merge October 8, 2025 07:30
@jl-wynen jl-wynen merged commit c0a6aeb into main Oct 8, 2025
4 checks passed
@jl-wynen jl-wynen deleted the bragg-peak-monitor-2 branch October 8, 2025 07:33
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.

4 participants