[ESSREFLECTOMETRY] feat: freia choppers and more scaffolding by jokasimr · Pull Request #747 · scipp/ess

jokasimr · 2026-09-11T14:19:08Z

No description provided.

…flow

jokasimr · 2026-09-17T10:04:04Z

+   "source": [
+    "freia_mcstas = freia.FreiaMcStasWorkflow(wavelength_from='analytical')\n",
+    "freia_mcstas[Filename[SampleRun]] = '../../../265305.h5'\n",
+    "freia_mcstas[KeepEventTimeOffset] = True\n",


I'll upload these files to the pooch repo

jokasimr · 2026-09-17T10:04:35Z

+   "source": [
+    "workflow = FreiaMcStasWorkflow(run_norm=RunNormalization.monitor_histogram)\n",
+    "workflow[Filename[SampleRun]] = '../../../265305.h5'\n",
+    "workflow[Filename[ReferenceRun]] = '../../../265301.h5'\n",


I'll upload these files or similar to the pooch repo.

nvaytet · 2026-09-17T11:37:30Z

-    :
-        Qx-values.
+    Approximate the flight time using the straight sample-to-detector distance,
+    as in ScippNeutron's gravity correction.


ScippNeutron's gravity correction.

Is this something that is already implemented in scippneutron or is it just an example copied from the docs?

If the former, can we import from scippneutron and use here instead of re-implementing?

nvaytet · 2026-09-17T11:53:05Z

+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "event_wavelengths = unwrapped.bins.constituents['data'].coords['wavelength']\n",


Can we do this differently than having to go via .bins.constituents?
Am I right in understanding that you want to remove the wavelengths that have been assigned NaN?

Can we instead do something like unwrapped.hist(wavelength=1).sum() and unwrapped.sum() to get the N events out of M?

Histogramming into 1 bin should discard all the NaNs.

This has been removed now 👍

nvaytet · 2026-09-17T11:54:19Z

+    "pp.plot(wavelength_image, norm='log', title='Wavelength across the detector', vmin=1e0)"
+   ]
+  }
+ ],


Is it worth adding at the end of the notebook an inspector plot, where we histogram in longitude, height and wavelength? And then draw some rectangles around some regions?

nvaytet · 2026-09-17T11:55:59Z

+    "\n",
+    "Compute one reflectivity curve for each incident beam using a sample measurement and a direct-beam measurement taken without a sample. Normalize by an incident wavelength monitor, select matching peaks, and compare the three curves with the silicon reflectivity reference.\n",
+    "\n",
+    "The sample and direct-beam runs must have matching slit and chopper settings. This example uses local files with WFM chopper settings."


This example uses local files with WFM chopper settings.

Not sure what "WFM chopper settings mean". Does it mean we use results from runs that were recorded in WFM mode? or data was recorded while WFM choppers were in use?

I don't understand the distinction? The Freia instrument has more or less 2 chopper modes. One high resolution mode where the WFM choppers are in use, and one where they are not in use. The McStas model can simulate both modes. In this case the data comes from a simulation where the WFM choppers are active.

Does that answer the question?

Hmm it was badly formulated from my part. I think I just repeated myself twice (probably I was editing my comment and it went wrong).

What I meant to say is that "WFM chopper settings" is unclear to the reader I think. In addition, I think we don't care that the files are 'local'?
Instead we should write something like "we use results recorded from runs where the WFM choppers were in use".

nvaytet · 2026-09-17T11:56:21Z

+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "%matplotlib inline\n",


Why do we need this?

I'll remove it 👍

nvaytet · 2026-09-17T12:20:55Z

-    """Corrects the data by the size of the footprint on the sample."""
-    return da / sc.sin(da.coords['theta'])

+def prepare_sample(


'prepare' is a little vague. Can we use a name which is more descriptive?

nvaytet · 2026-09-17T12:29:04Z

+        low, high = low.to(unit=coord.unit), high.to(unit=coord.unit)
+        if not (low <= high).value:
+            raise ValueError(f'Reversed ROI bounds for {name!r}.')
+        (event_masks if is_event_coord else masks)[f'roi_{name}'] = ~(


So here, if you have data which is binned in e.g. 'x' and 'y', 'x' and 'y' will be found both in the da.coords and in the da.bins.coords. If I followed correctly, we are first looking into the .bins.coords and if it's there, putting a mask on each event.

However, in that case it would be much cheaper to mask the bins rather than the events.
Should we first check if the key is found in the da.coords?

I'm not sure about this. The results of masking the bins and masking the events are not the same.
When the coord exists on both the binning level and the event level then the intention of the masking is ambiguous, but my guess is that applying the masking to the event coord is what most users would understand as "correct".

In this case there is no ambiguity though, because the function is not applied to data that has been binned by event coordinates, the data only has a pixel binning. But of course that could change if some earlier step in the workflow changes.

the intention of the masking is ambiguous, but my guess is that applying the masking to the event coord is what most users would understand as "correct".

Do you mean that it's the difference between masking an entire pixel, and masking only the events inside the ROI if, for example, the edge of the drawn rectangle passes through the middle of the pixel? If we mask on the upper level, we would have to decide whether to include or exclude pixels that are partially covered by the ROI.
If we mask the events themselves, we don't need to worry about that.

Was that what you meant?

Yes, I mean masking the binning and masking the events does not return the same result.
It only returns the same result is if the masking interval coincides with the bin-edges of the binning, or if the coordinate is constant within the bins, which is unusual.

I think it's typically more correct to mask the events, because the bin-edges are derived from the event coords, the event coords is the ground truth, but ultimately I think what is correct depends on context.

But this ambiguity only occurs when there is a coordinate with the same name on the binning and in the events, and that is not the case where this mask function is applied.

nvaytet · 2026-09-17T12:32:26Z

+    )
+
+
+def _detector_geometry(data, detector_name) -> sc.DataArray:


The best thing here would have been to put the mcstas events inside a coda skeleton for Freia. Then we would avoid a lot of this code...

nvaytet · 2026-09-17T12:33:19Z

+    normal = direct_beam.coords['sample_surface_normal']
+    normal = normal / sc.norm(normal)
+    outgoing = direct_beam.bins.coords['outgoing_direction']
+    reflected = outgoing - 2 * sc.dot(outgoing, normal) * normal


Can you explain or add a link as to where these formulas come from?

Mirroring the outgoing direct beam in the sample plane produces the outgoing beam if it had been reflected. That tells us what (angle of reflection) theta bin and correspondingly what Q bin it should be assigned to when we compare the direct beam intensity to the reflected intensity.

The explanations should go close to the code (either comment or docstring)

…th-lookup-table.ipynb Co-authored-by: Neil Vaytet <[email protected]>

…flow

jokasimr · 2026-09-18T12:22:57Z

I think a lot of the comments here are good, but they are not critical. Can I collect them to a separate issue and finish that up later?

Then we can merge and deploy this and we have something to show for Tuesday.

nvaytet · 2026-09-21T07:01:38Z

Then we can merge and deploy this and we have something to show for Tuesday.

So you would like to do this because you'd prefer not to work from a branch when you do the demo?

jokasimr · 2026-09-21T07:36:29Z

So you would like to do this because you'd prefer not to work from a branch when you do the demo?

Mainly because I think it would be nice to have docs deployed so that if someone wants to take a look themselves they can at least find something there.

But ofc if there are some high-priority issues to fix, I can definitely do that first.

nvaytet · 2026-09-21T08:23:14Z

But ofc if there are some high-priority issues to fix, I can definitely do that first.

No, I agree that most comments were about naming or things in the notebooks.
Can you just fix the issue with the masking which goes on the events when it doesn't need to?
And then collect the rest in an issue?

Note that we don't need to release to get the docs online, we can merge into main and run the docs workflow manually, checking the 'publish' box.

jokasimr · 2026-09-21T08:48:30Z

Note that we don't need to release to get the docs online, we can merge into main and run the docs workflow manually, checking the 'publish' box.

Yeah but I think it's better that the docs matches the release? I mean, if anyone tries anything out they will get errors otherwise and things will seem broken.

Can you just fix the issue with the masking which goes on the events when it doesn't need to?
And then collect the rest in an issue?

I'll do that 👍

jokasimr · 2026-09-21T11:50:16Z

Created an issue to do the followup here: #756

feat: freia choppers and more scaffolding

7497ec3

github-actions Bot added the essreflectometry Issues for essreflectometry. label Sep 11, 2026

github-actions Bot changed the title ~~feat: freia choppers and more scaffolding~~ [ESSREFLECTOMETRY] feat: freia choppers and more scaffolding Sep 11, 2026

jokasimr added 2 commits September 11, 2026 16:26

docs: fix tone and content, remove unnecessary

38cadda

feat: normalizations by monitor, footprint and direct beam

d5a6850

jokasimr commented Sep 15, 2026

View reviewed changes

jokasimr added 4 commits September 15, 2026 16:55

Merge branch 'main' into freia-workflow

1eb5205

fix: clean up

1dfbd69

docs: cleanup

1e38d75

Merge branch 'freia-workflow' of github.com:scipp/ess into freia-work…

09ccd45

…flow

jokasimr marked this pull request as ready for review September 15, 2026 16:00

jokasimr requested a review from nvaytet September 17, 2026 08:11

jokasimr added 3 commits September 17, 2026 11:59

refactor: rename domain type to common ess name

6adc090

refactor: rename in estia

9d86aa3

refactor: make structure more similar to other workflows

7cd3c91

jokasimr commented Sep 17, 2026

View reviewed changes

refactor: remove unnecessary loaders and domain types

8302aad

nvaytet reviewed Sep 17, 2026

View reviewed changes

jokasimr and others added 8 commits September 17, 2026 15:30

ci: upload files to pooch

8ef8a81

Update packages/essreflectometry/docs/user-guide/freia/freia-waveleng…

0fa59e7

…th-lookup-table.ipynb Co-authored-by: Neil Vaytet <[email protected]>

fix

4f90a0d

pixi lock

c3e329b

Merge branch 'freia-workflow' of github.com:scipp/ess into freia-work…

2d8a2a0

…flow

docs

5b6ccc5

docs

6126c9c

docs: remove unnecessary comment

028aed3

docs: cleanup and add wavelength error figure

f1bedb9

nvaytet approved these changes Sep 21, 2026

View reviewed changes

jokasimr added this pull request to the merge queue Sep 21, 2026

Merged via the queue into main with commit 18a194c Sep 21, 2026
24 checks passed

jokasimr deleted the freia-workflow branch September 21, 2026 14:23

jokasimr restored the freia-workflow branch September 22, 2026 12:06

		)


		def _detector_geometry(data, detector_name) -> sc.DataArray:

Conversation

jokasimr commented Sep 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokasimr Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokasimr Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jokasimr commented Sep 18, 2026

Uh oh!

nvaytet commented Sep 21, 2026

Uh oh!

jokasimr commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvaytet commented Sep 21, 2026

Uh oh!

jokasimr commented Sep 21, 2026

Uh oh!

jokasimr commented Sep 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jokasimr Sep 21, 2026 •

edited

Loading

jokasimr Sep 17, 2026 •

edited

Loading

jokasimr commented Sep 21, 2026 •

edited

Loading