Use sciline #7

jl-wynen · 2023-11-22T14:53:48Z

This is a first draft of a Sciline pipeline. Can you take a look if this is a proper usage of Sciline? I'm going to add tests and more documentation later.

I tested it using

import scipp as sc
import sciline
import ess.diffraction
from ess.diffraction.external import powgen
from ess.diffraction import powder
import scippneutron as scn
from ess.diffraction.grouping import group_by_two_theta, merge_all_pixels

from ess.diffraction.types import *

params = {
    CalibrationFilename: 'PG3_FERNS_d4832_2011_08_24.zip',
    DspacingBins: sc.linspace('dspacing', 0.0, 2.3434, 200, unit='angstrom'),
    Filename[SampleRun]: 'PG3_4844_event.zip',
    Filename[VanadiumRun]: 'PG3_4866_event.zip',
    TwoThetaBins: sc.linspace(dim='two_theta', unit='deg', start=25.0, stop=90.0, num=16),
    ValidTofRange: sc.array(dims=['tof'], values=[0.0, 16666.67], unit='us'),
}
providers = [
    *ess.diffraction.providers,
    *powder.providers,
    *powgen.providers,
]
# providers.remove(merge_all_pixels)
# providers = (*providers, group_by_two_theta)
pl = sciline.Pipeline(
    providers,
    params=params,
)
result = pl.compute(DspacingHistogram)

And compared to https://scipp.github.io/ess/instruments/external/powgen/powgen_reduction.html#Vanadium-correction
The results don't quite agree. This is because the old workflow did not crop the vanadium data in tof but the new pipeline does. Without this change, the results are identical.

SimonHeybrock · 2023-11-23T06:02:19Z

Looks good. Can you show the resulting graph?

The results don't quite agree. This is because the old workflow did not crop the vanadium data in tof but the new pipeline does. Without this change, the results are identical.

Why was the cropping added? Is this a new requirement, or was it an oversight in the old workflow?

jl-wynen · 2023-11-23T08:30:53Z

Why was the cropping added? Is this a new requirement, or was it an oversight in the old workflow?

I think it was an oversight in the old workflow. I see no reason why the vanadium run would be able to access a higher tof range. In Mantid, the tof bounds are obtained from an isntrument characterization file. Since we don't have those, I hard coded the bounds in the workflow. But then forget to apply them to vanadium.

jl-wynen · 2023-11-23T08:35:51Z

SimonHeybrock · 2023-11-23T09:06:12Z

👍

jl-wynen · 2023-11-29T13:11:58Z

Ready now. Fixes #3

SimonHeybrock · 2023-12-05T10:10:29Z

src/ess/diffraction/correction.py

+    data: MergedPixels[SampleRun],
+    vanadium: MergedPixels[VanadiumRun],


I think in powder, the reduction (pixel merging) is referred to as "focussing", I don't know if that name should be adopted here?

SimonHeybrock · 2023-12-05T10:14:25Z

src/ess/diffraction/filtering.py

+    return TofCroppedData[RunType](
+        data.bin(tof=tof_range.to(unit=data.coords['tof'].unit))
+    )


Note https://scipp.github.io/user-guide/binned-data/filtering.html#Extract-events-falling-into-a-parameter-interval, which would avoid the need for tof binning, i.e., adding tof as a dim.

This doesn't work. It seems to do nothing:

You forgot a .bins

SimonHeybrock · 2023-12-05T10:14:42Z

src/ess/diffraction/filtering.py

+    :
+        `data` with bad events removed.
+    """
+    # TODO this needs to filter by proton charge once we know how


Is this a new or old todo?

It wasn't written down before. I added this provider in order to define a FilteredData type and make the tof-cropping explicit in the graph. Alternatively, I could drop this provider and make crop_tof return FilteredData.

SimonHeybrock · 2023-12-05T10:17:00Z

src/ess/diffraction/grouping.py

+def finalize_histogram(
+    data: NormalizedByVanadium, edges: DspacingBins
+) -> DspacingHistogram:


This looks like we are normalizing in event mode, which is bad for error handling. Should histogram before vanadium division?

Maybe. I didn't want to alter the workflow.

Can you open an issue at least?

SimonHeybrock · 2023-12-05T10:18:41Z

src/ess/diffraction/types.py

+TwoThetaBins = NewType('TwoThetaBins', sc.Variable)
+"""Bin edges for 2theta."""


I suppose this is for grouping by scattering angle, instead of into a single spectrum? Can you document this?

SimonHeybrock · 2023-12-05T10:19:15Z

src/ess/diffraction/types.py

+# This is Mantid-specific and can probably be removed when the POWGEN
+# workflow is removed.
+DetectorInfo = NewType('DetectorInfo', sc.Dataset)
+"""Mapping between detector numbers and spectra."""


Should it be moved into external/powgen then?

SimonHeybrock · 2023-12-05T10:19:51Z

src/ess/diffraction/types.py

+    """Raw data."""
+
+
+class RawDataWithvariances(sciline.Scope[RunType, sc.DataArray], sc.DataArray):


Suggested change

class RawDataWithvariances(sciline.Scope[RunType, sc.DataArray], sc.DataArray):

class RawDataWithVariances(sciline.Scope[RunType, sc.DataArray], sc.DataArray):

SimonHeybrock · 2023-12-05T10:20:35Z

src/ess/diffraction/uncertainty.py

+def drop_variances(data: RawDataWithvariances[VanadiumRun]) -> RawData[VanadiumRun]:
+    res = data.copy(deep=False)
+    if res.bins is not None:
+        res.bins.constituents['data'].variances = None
+    else:
+        res.variances = None
+    return RawData[VanadiumRun](res)
+
+
+providers = (drop_variances,)
+"""Sciline providers for handling statistical uncertainties."""


Why are the vanadium errors dropped?

Because we normalise in event mode and that requires broadcasting a vanadium histogram to sample events.

Maybe we should not do that?

SimonHeybrock · 2023-12-05T10:23:44Z

docs/examples/POWGEN_data_reduction.ipynb

+    "    *ess.diffraction.providers,\n",
+    "    *powder.providers,\n",


This looks a bit confusing, powder is a submodule of ess.diffraction? What is the split? Do you want to explain to the reader?

I thought the goal was to also have single crystal code in this project. So I kept the old module structure from ess.

So there are things that are common between powder and SXD?

I don't know. I thought that was the reason you suggested a common package.

My point is: You have ess.diffraction.providers, implying there is something common?

Yes. Many providers are regular functions that were already defined in the common diffraction part of ess. I don't know whether they can be used by SXD. Esp. as providers because the domain types are probably all different.

So what do you suggest? Move everything into powder? Or remove powder and move everything into diffraction?

I think you can leave it, but maybe add a comment?

Opened an issue to track this and make us re-examine when we add sxd code.

SimonHeybrock · 2023-12-05T10:24:40Z

docs/examples/POWGEN_data_reduction.ipynb

+    "\n",
+    "We ultimately need to write the reduced data to a file.\n",
+    "This could be done with the `result` we computed above.\n",
+    "But then we would have to provide all metadata manually.\n",


I suppose there is no metadata here? This is a bit confusing maybe?

The filename, sort of. But yes, there isn't any real metadata. I can change it now and revert later when we write a cif file instead.

SimonHeybrock · 2023-12-05T10:26:19Z

docs/examples/POWGEN_data_reduction.ipynb

+    "The above pipeline merges all instrument pixels to produce a 1d d-spacing curve.\n",
+    "If instead we want to group into $2\\theta$ bins, we can alter the pipeline by replacing the grouping step:"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "d24c8bf8-ad59-4211-ae6a-3ee29b0556a3",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "from ess.diffraction.grouping import group_by_two_theta, merge_all_pixels\n",
+    "\n",
+    "grouping_providers = list(providers)\n",
+    "grouping_providers.remove(merge_all_pixels)\n",
+    "grouping_providers = (*grouping_providers, group_by_two_theta)"


Would it make more sense to have TwoThetaBins as an optional input to a single provider, which falls back to merging all pixels of no bins provided? This would avoid the need to modify the provider list.

Maybe. What I don't like about it (and the current approach) is that the dims of the result are different. So downstream providers cannot necessarily use the data. Swapping a providers is more explicit about this change than adding a parameter.

But maybe the two providers should return different types?

Yes, swapping a provider does not fix the problem, so a different type would work.

According to my discussion with Andrew about CIF, it seems that we will have to focus in some way that still needs to be determined. It sounds like grouping all pixels is quite useless. But using 2theta is likely also not a good approach, at least for DREAM.

So I wouldn't put too much thought into finding a good solution now. Since this is not the last step in the pipeline, I'd rather keep the unified output type until we know more.

jl-wynen · 2023-12-06T13:50:07Z

I think I addressed your comments. Can you have another look, @SimonHeybrock ?

jl-wynen added 4 commits November 21, 2023 15:42

Begin implementing Sciline providers

ab8df4a

Pipeline up to conversion to dspacing

5dc6ba5

Full pipeline

d2c7bc4

Add grouping in two_theta

555557a

jl-wynen added 6 commits November 24, 2023 16:37

Convert docs to use sciline

4c8e7a6

Write reduced data to disk

663fb59

Add missing objects to docs

5baca32

More docstrings

44cb057

Add tests of pipeline

38727ac

del internal names

9c86b50

jl-wynen marked this pull request as ready for review November 29, 2023 10:20

jl-wynen requested a review from SimonHeybrock November 29, 2023 10:20

SimonHeybrock reviewed Dec 5, 2023

View reviewed changes

jl-wynen added 6 commits December 5, 2023 15:58

merge -> focus

9f83d41

Explain TwoThetaBins

a656d3e

Move DetectorInfo into powgen.types

a7dcc40

Plot FilteredData consistently

bcdcb3f

Fix typo

e2cbb6f

Do not mention metadata

87089d1

jl-wynen mentioned this pull request Dec 6, 2023

Package structure for subtechniques #9

Closed

SimonHeybrock approved these changes Dec 7, 2023

View reviewed changes

jl-wynen merged commit c54508a into main Dec 7, 2023
3 checks passed

jl-wynen deleted the use-sciline branch December 7, 2023 08:09

		data: MergedPixels[SampleRun],
		vanadium: MergedPixels[VanadiumRun],

		TwoThetaBins = NewType('TwoThetaBins', sc.Variable)
		"""Bin edges for 2theta."""

		"""Raw data."""


		class RawDataWithvariances(sciline.Scope[RunType, sc.DataArray], sc.DataArray):

Use sciline #7

Use sciline #7

Conversation

jl-wynen commented Nov 22, 2023

SimonHeybrock commented Nov 23, 2023

jl-wynen commented Nov 23, 2023

jl-wynen commented Nov 23, 2023

SimonHeybrock commented Nov 23, 2023

jl-wynen commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jl-wynen commented Dec 6, 2023