Estia #108

jokasimr · 2024-12-10T13:46:22Z

Fixes #91

Initial draft for the ESTIA data reduction workflow.

nvaytet · 2025-02-28T08:28:36Z

src/ess/estia/__init__.py

+
+providers = (
+    *reflectometry_providers,
+    # *load.providers,


Suggested change

# *load.providers,

nvaytet · 2025-02-28T08:35:18Z

src/ess/estia/conversions.py

+    """
+    p = position - sample_position.to(unit=position.unit)
+    return (
+        sc.atan2(y=p.fields.x, x=p.fields.z)


Suggested change

sc.atan2(y=p.fields.x, x=p.fields.z)

sc.atan2(y=p.fields.y, x=p.fields.z)

Not sure I followed everything, but I am wondering if it should be y here?
Are we talking about this figure:

Amor scatters vertically but Estia scatters horizontally, that's what makes the difference.

src/ess/estia/conversions.py

nvaytet · 2025-02-28T08:36:06Z

src/ess/estia/conversions.py

+
+    Computes the angle between the scattering direction of
+    the neutron and the sample surface.
+    '''


Can you also add descriptions of the parameters in the docstring?

nvaytet · 2025-02-28T08:36:26Z

src/ess/estia/conversions.py

+def divergence_angle(
+    position, sample_position, detector_rotation, incident_angle_of_center_of_beam
+):
+    """
+    Angle between the scattering direction and
+    the ray from the sample to the center of the detector.
+    """


Type hints and params in docstring.

nvaytet · 2025-02-28T09:42:05Z

tests/estia/mcstas_test.py

+    return wf
+
+
+def test_mcstas_loading(estia_mcstas_pipeline: sciline.Pipeline):


Seems this test is doing a bit more than loading. It's also adding a bunch of coordinates.

nvaytet · 2025-02-28T09:43:33Z

tests/estia/mcstas_test.py

+    assert 'detector_rotation' in da.coords
+    assert 'theta' in da.coords
+    assert 'wavelength' in da.bins.coords
+    assert 'Q' in da.bins.coords


So Q is already in the coordinates? I always thought that computing Q was the goal of the reduction?

Computing the reflectivity as a function of Q is the goal of the reduction. But Q is just a coordinate associated with every event like wavelength or anything else.

nvaytet · 2025-02-28T09:44:20Z

tests/estia/mcstas_test.py

+    assert 'Q' in da.bins.coords
+
+
+def test_can_compute_reflectivity_curve(estia_mcstas_pipeline: sciline.Pipeline):


Out of curiosity, how long does this test take to run?

I already trimmed the McStas files quite a bit, so it's fast.

nvaytet · 2025-02-28T09:45:09Z

tests/estia/mcstas_test.py

+    ).nanmax()
+
+    assert max_q > sc.scalar(0.075, unit='1/angstrom')
+    assert min_q < sc.scalar(0.007, unit='1/angstrom')


I feel we could benefit from more tests which check that the different parameters do what they are supposed to do (e.g. changing the masks or something else).
But it's also ok to do that in a follow-up PR.

I agree it would be good to have more tests.

nvaytet · 2025-02-28T09:45:49Z

tests/estia/mcstas_test.py

@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (c) 2024 Scipp contributors (https://github.com/scipp)
+# flake8: noqa: F403, F405


Why the global flake8?
Also, I thought we had moved to ruff everywhere?

Don't know, I copied the file from one of the amor tests. I'll try to remove it here and from the amor tests.

nvaytet

Couple of remaining things (summarizing here as it can get lost in the noise):

move masking to a separate file
unresolved (?) discussion about variances on mcstas data (waiting for simon to reply)

Otherwise, I think everything else has been addressed?

src/ess/estia/conversions.py

Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>

…stent with estia

jokasimr · 2025-03-12T08:13:53Z

I'll add an example notebook for reducing Estia McStas data

… before normalizing

nvaytet · 2025-03-14T09:11:13Z

docs/user-guide/estia/estia-mcstas-reduction.ipynb

+    "q_theta_figure(results, q_bins=300, theta_bins=200)"
+   ]
+  }
+ ],


The notebook makes a lot a plots. I think it would really benefit from some short explanations between the figures, explaining what we are showing.

I agree with you, but I think it's good to not make this PR any longer now. It's a basic notebook that someone experienced in the technique will at least get the gist of. Let's postpone this step to a separate PR for improving the docs.

I disagree, I think leaving it til later will increase the chances of us forgetting.

I don't think it's so much work to add small sentences and sections explaining a little what we are doing, especially if this is going to appear in the docs.

There are some explanations already added, maybe take a look at those and see if that's what you wanted to see there.

Yes, sorry I had missed those.

…n the notebook

SimonHeybrock · 2025-03-17T08:45:09Z

* unresolved (?) discussion about variances on mcstas data (waiting for simon to reply)

@nvaytet You are the main reviewer here, if you are confident that it is correct and adequately explained, go ahead!

jokasimr force-pushed the estia branch from ff27626 to 8d08727 Compare February 5, 2025 08:45

jokasimr force-pushed the estia branch from 8c6718f to df83bfe Compare February 27, 2025 09:24

jokasimr marked this pull request as ready for review February 27, 2025 09:24

jokasimr requested a review from nvaytet February 27, 2025 09:38

jokasimr mentioned this pull request Feb 27, 2025

Polarization correction #122

Open

nvaytet reviewed Feb 28, 2025

View reviewed changes

nvaytet reviewed Mar 4, 2025

View reviewed changes

src/ess/estia/conversions.py Outdated Show resolved Hide resolved

jokasimr and others added 23 commits March 11, 2025 12:52

Create estia-data-reduction.md

a0abff8

feat: initial setup for estia workflow

0dff310

feat: compute angle without divergence angle per pixel

91a69cb

fix: dont modify input + dont keep intermediates

62b654c

fix: remove operation modifying input

c0cd69e

feat: estia coordinate transformations + workflow

0af30e9

WIP: estia workflow - changes to the generic part of the workflow

29c983f

WIP: estia workflow

f594a68

fix

ee1b3d8

wip

b8ef151

feat: add mcstas h5 file loader

09b50a4

fix: allow loading ascii and h5 files

0032aa1

Apply automatic formatting

4666e24

fix: reshape to expected dimensions

696668e

test: add mcstas test files and tests

83406c2

minor

39422bf

docs: removing from this pr because documentation needs some rewriting

6afc1eb

refactor: share parts of the workflow between amor and estia

6b21a1b

fix: remove comment and add providers to load

32c6a46

docs: typehints

98ff25c

docs: add parameters in docstrings

0a2946e

fix: mod t to get event_time_offset + remove unnecessary coordinates

9c88841

docs: update licence headers

761233e

jokasimr and others added 2 commits March 11, 2025 12:55

Update src/ess/estia/conversions.py

539b9ae

Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>

doc: add comment clarifying mcstas weight variances

f23e91e

jokasimr force-pushed the estia branch from c7acfdb to f23e91e Compare March 11, 2025 11:55

jokasimr added 4 commits March 11, 2025 13:15

refactor: remove 'center of incoming' concept from amor to make consi…

5d8f3d8

…stent with estia

fix: better guess

ecc2dac

fix: allows passing a number as QBins

ddf7ec3

docs: add variance comment to ascii mcstas loader

dd02ed5

jokasimr added 6 commits March 12, 2025 15:55

refactor: move figures to common reflectometry module

e012cd0

fix: load required data from mcstas hdf5 output stored in attributes

78a87cf

Merge remote-tracking branch 'origin/main' into estia

8b72a05

fix: handle when theta is in outer coords, accept integer bins

3dd2295

docs: add example datasets

8c2097b

docs: mcstas data redcution examples

e304f2a

jokasimr requested a review from nvaytet March 13, 2025 14:48

jokasimr added 6 commits March 13, 2025 16:45

fix: keep variances on reference, to make it nicer to inspect, remove…

6ec0b06

… before normalizing

docs: clean up notebook

16c43cf

docs: clean up notebook

d283075

fix

54bd312

docs: add estia section to docs

19ab5b9

docs: update api reference page

c48ba09

nvaytet reviewed Mar 14, 2025

View reviewed changes

jokasimr added 4 commits March 14, 2025 13:42

docs: move modules in docs that have been moved in the code

8c0cf0f

docs: add ground truth reflectivity comparison and some explanation i…

1d0f2d4

…n the notebook

fix

ae42bb5

fix

413ed1a

nvaytet approved these changes Mar 18, 2025

View reviewed changes

jokasimr merged commit 2706ec5 into main Mar 18, 2025
4 checks passed

jokasimr deleted the estia branch March 18, 2025 14:03

	sc.atan2(y=p.fields.x, x=p.fields.z)
	sc.atan2(y=p.fields.y, x=p.fields.z)

		return wf


		def test_mcstas_loading(estia_mcstas_pipeline: sciline.Pipeline):

		assert 'Q' in da.bins.coords


		def test_can_compute_reflectivity_curve(estia_mcstas_pipeline: sciline.Pipeline):

Estia #108

Estia #108

Uh oh!

Conversation

jokasimr commented Dec 10, 2024

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!

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!

nvaytet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jokasimr commented Mar 12, 2025

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!

SimonHeybrock commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!