SANS Loki basic workflow #60

wpotrzebowski · 2021-11-02T05:39:11Z

Basic workflow for LoKI demonstarted on SANS2D data.

…one folder for convienece at the moment

SimonHeybrock

Thanks for this addition! A number of comments below (if have not looked at the notebook yet). I think either @nvaytet or myself would pair-program with you to address some potentially more complex bits, and to ensure that this is in line with our latest guidelines (for example, helping in setting up unit tests).

src/ess/loki/load_files.py

SimonHeybrock · 2021-11-03T08:44:49Z

src/ess/loki/load_files.py

+        sample.masks[f"mask_{i}_xml"] = mask_xml["spectrum", :spectrum_size].data
+        background.masks[f"mask_{i}_xml"] = mask_xml["spectrum", :spectrum_size].data


Looks SANS2D specific again (with the spectrum_size argument)?

Is there a more sensible mask name than just enumerating? Maybe parts of the filename?

src/ess/sans/contrib.py

src/ess/sans/sans.py

src/ess/sans/contrib.py

nvaytet

Some initial thoughts/comments.

nvaytet · 2021-11-03T10:22:04Z

src/ess/loki/load_files.py

+import scippneutron as scn
+
+
+def load_isis(filename, spectrum_size, tof_bins):


You could put these functions into an io submodule

src/ess/loki/sans2d/SANS2D_reduction.ipynb

nvaytet · 2021-11-03T10:25:01Z

src/ess/loki/sans2d/SANS2D_reduction.ipynb

+    "    !python make_config.py -h # change to `-f path` or run in terminal\n",
+    "\n",
+    "path = dataconfig.data_root\n",
+    "direct_beam_file = 'DIRECT_SANS2D_REAR_34327_4m_8mm_16Feb16.dat'\n",


All these files should be put somewhere so that the notebook can find them so it can run. We should put them on the dmsc web server.

nvaytet · 2021-11-03T10:27:30Z

src/ess/loki/sans2d/SANS2D_reduction.ipynb

+    "background_transmission_run_number = 63159 \n",
+    "direct_run_number = 63091\n",
+    "\n",
+    "mask_1 = f'{path}/MASK_SANS2D_REAR_Edges_16Mar2015.xml'\n",


I don't really understand the point of having all these masks in xml files.
From the names, it seems they are masking either tubes or physical regions in space.

We should be able to come up with a programmatic way of masking the tubes, maybe if we reshape the data into logical (tube,straw,pixel) dimensions?

Maybe some masking based on count values would also be more compact?

I agree, but I believe this is simply for reproducing specific SANS2D results, so maybe ok for that, as long as we ensure that for ESS instruments a better mechanism is put in place?

nvaytet · 2021-11-03T10:37:53Z

src/ess/loki/sans2d/SANS2D_reduction.ipynb

+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "setup_offsets(sample, sample_trans, background, background_trans, direct, sample_pos_z_offset, bench_pos_y_offset, monitor4_pos_z_offset)"


I think the interface to this function is slightly odd. It assumes you always have the same list of input DataArrays, which hampers portability.
The function should probably just take in a single DataArray to apply corrections on, and then you call the function on the DataArrays you wish to correct.

In addition, have you thought about putting your data in a Dataset, so you can do these kind of things in a single call?

nvaytet · 2021-11-03T10:50:16Z

src/ess/loki/sans2d/transform_coordinates.py

+import scipp as sc
+
+
+def setup_offsets(


Change to accept only one input data array.
You should also add type hints to the arguments

src/ess/sans/reduction.py

src/ess/sans/sans.py

nvaytet · 2021-11-03T10:52:59Z

src/ess/sans/sans.py

+from .normalization import solid_angle, transmission_fraction
+
+
+def to_wavelength(


Can we do this in a more succint way via transform_coords?

Maybe not, because this is essentially doing half of a SANS data reduction?

wpotrzebowski · 2021-11-04T07:20:51Z

@SimonHeybrock @nvaytet thanks for useful comments! I will start off from addressing obvious cases and then indeed pair programing session(s) would be very useful. I can also see some more open questions e.g. xml mask files that may require some broader discussion but I guess we can take it at the later stage.

SimonHeybrock

Just a few minor more comments/questions from my side:

SimonHeybrock · 2021-11-29T04:16:59Z

src/ess/loki/load_sans2d.py

+    idf_filename,
+    mask_files,
+    data_set,
+    spectrum_size,


Why is this parameter required? Isn't it just data_set.sizes['spectrum']?

Note that we typically use ds as a generic variable names for datasets (and da for data arrays).

src/ess/sans/contrib.py

src/ess/sans/normalization.py

SimonHeybrock · 2021-11-29T04:23:14Z

src/ess/sans/normalization.py

+
+def covert_and_rebin(data, wavelength_bins, begin, end):
+    """
+    Convertining and rebinning shifted data


Suggested change

Convertining and rebinning shifted data

Convertining and rebinning shifted data

This docstring does not say more than the function name? What is the actual function purpose?

Before this function was called setup and I thought I can give it more descriptive name. Can revert it to setup if it works better.

I think if a function is just used within a file then a good name may not be so important, but it seems this is imported elsewhere (sans.py) so we probably want something that is truly descriptive?

Indeed, it was initially used internally in one file but then it turned out that can be applied elswhere. Maybe contrib.py is anyway better place for such helper function?

contrib.py was menat for things that may eventuallt make it into scipp itself. I think that is quite unlikely for this particular function.

src/ess/sans/sans.py

SimonHeybrock · 2022-02-04T05:33:11Z

src/ess/sans/normalization.py

+        background = (below.sum() + above.sum()) / sc.scalar(below.sizes[dim] +
+                                                             above.sizes[dim])


What if someone puts a mask on a monitor?

SimonHeybrock · 2022-02-04T05:44:38Z

src/ess/sans/normalization.py

+    :param dim: In the case of a numerator containing event data, this is the dimension
+        along which the lookup operation should be performed. This can be omitted in
+        case the denominator has only one dimension.


I appreciate the idea to keep this flexible, but is this ever not Q in SANS?

ok, I hard-coded the Q in the function body instead.

SimonHeybrock

A few extra comments from reviewing the notebooks:

SimonHeybrock · 2022-02-04T09:44:26Z

docs/instruments/loki/sans2d_to_I_of_Q.ipynb

+    "tof_masked_region = sc.concat([sample.coords['tof']['tof', 0],\n",
+    "                               mask_tof_min, mask_tof_max,\n",
+    "                               sample.coords['tof']['tof', -1]], dim='tof')\n",


This assumes event data. Maybe mention this on top of the notebook?

SimonHeybrock · 2022-02-04T09:47:58Z

docs/instruments/loki/sans2d_reduction.ipynb

+    "\n",
+    "**Note:** We use programatic masks here and not those stored in xml files.\n",
+    "\n",
+    "# Mask bad pixels\n",


Suggested change

"# Mask bad pixels\n",

"### Mask bad pixels\n",

?

SimonHeybrock · 2022-02-04T09:51:14Z

docs/instruments/loki/sans2d_reduction.ipynb

+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "result = sample_q.bins.sum() - background_q.bins.sum()\n",


Two notes here:

Instead of sum() once could use histogram() here to define different Q bins.

This can be done in event mode, see https://scipp.github.io/user-guide/binned-data/computation.html#Subtraction.

I thought I would keep the same binning as was requested at the top of the notebook?

Possibly, but there may be performance advantages to use a coarse Q binning when the computing I(Q) numerator. Not saying the change this now, but you may put a note, maybe?

SimonHeybrock · 2022-02-04T09:52:41Z

docs/instruments/loki/sans2d_reduction.ipynb

+    "    data_incident_monitor=binned['background'].attrs[\"monitor2\"].value,\n",
+    "    data_transmission_monitor=binned['background'].attrs[\"monitor4\"].value,\n",
+    "    direct_incident_monitor=binned['direct'].attrs[\"monitor2\"].value,\n",
+    "    direct_transmission_monitor=binned['direct'].attrs[\"monitor4\"].value,\n",


This is repeated many times. How about changing the functions to take a dict of monitors as input?

Then we would be relying on special keys inside the to_I_of_Q function?

Well, you are anyway relying on special function parameter names?

I added dicts, and used recursion in some functions to make them accept dict for convenience.
I think it works quite nicely.

SimonHeybrock · 2022-02-04T13:55:32Z

src/ess/sans/i_of_q.py

+    if isinstance(data, dict):
+        return {


Note sure about having this recursion everywhere?

ok i'll remove it here

I meant everywhere. Can this be solved in a more Pythonic way, without having to handle dicts in every function?

…basic_workflow

nvaytet · 2022-02-04T14:03:13Z

@SimonHeybrock
Wojciech says to wait before merging, as he needs to be sure that we can have the data publicly available online...

wpotrzebowski · 2022-02-04T21:31:21Z

It is fine to use the data. I will provide reference to the publication.

wpotrzebowski · 2022-02-05T05:57:35Z

Reduced data has been published in https://aip.scitation.org/doi/full/10.1063/5.0059238 (it is one of data sets in Fig5d - hd-DES_10_h-C16EO8). It should be sufficent to reference the paper

nvaytet · 2022-02-07T09:09:31Z

@wpotrzebowski can you have a quick look to see if the way I added the citation is ok?
Thanks

…basic_workflow

Wojtek Potrzebowski added 17 commits July 2, 2021 10:41

Moving loading and transforming coordinates to separate files

c279e71

Towards further modularization of SANS module. Keeping everything in …

0042f12

…one folder for convienece at the moment

sans files updates

9847fd7

Further modularization

87eb386

Updates in notebook

b276772

few fixes and checks for SANS2D data processing

48164f9

Minor fixes

ffbb213

Few changes to further explore wavelenght slices

f0210db

fixes to SANS2D scripts

a86241b

moved parameters from function

b79a55c

Mask loading simplified

7a3e852

code clean-up

b7ce775

Fixing formating

b6485da

Files moved arround

ca779a7

Modules move arround

9d080db

modules fixes

5ecbe3e

fixing arguments for TOF bins

3154e40

wpotrzebowski marked this pull request as draft November 2, 2021 05:40

Wojciech Potrzebowski and others added 3 commits November 2, 2021 06:41

Merge branch 'main' into loki_basic_workflow

17ecf33

Cleaning main notebook

e84b383

further clean up on Notebook

8451e98

SimonHeybrock reviewed Nov 3, 2021

View reviewed changes

nvaytet reviewed Nov 3, 2021

View reviewed changes

Wojtek Potrzebowski added 5 commits November 8, 2021 15:45

Few fixes to address PR comments, TBC]

2e7a2a3

Fixing sans and contrib file

4337d6a

Fixes to notebook

8e3181b

Removing code duplication

b858abe

Introducing data set

558ef05

SimonHeybrock reviewed Nov 29, 2021

View reviewed changes

Apply automatic formatting

69aa4a7

SimonHeybrock reviewed Feb 4, 2022

View reviewed changes

hard-code Q dim in normalize

e01665f

SimonHeybrock reviewed Feb 4, 2022

View reviewed changes

nvaytet and others added 11 commits February 4, 2022 10:54

compute background mean taking into account potential masks

e018477

remove math formatting from notebook titles

22643d2

use a gravity parameter in the notebooks

1bd347c

add docstrings to helper functions in to_I_of_Q

5e26323

use dicts for monitors instead of many arguments

383e27e

update python tests

ee22d9f

cleanup

1cbf50b

use recursion on convsertion to Q also

efbb5a5

add i_of_q helpers to API reference

3f68526

flake8

64153a8

Apply automatic formatting

8ce7bc9

SimonHeybrock reviewed Feb 4, 2022

View reviewed changes

SimonHeybrock approved these changes Feb 4, 2022

View reviewed changes

nvaytet added 2 commits February 4, 2022 14:59

remove recursion on Q conversion

a01debd

Merge branch 'loki_basic_workflow' of github.com:scipp/ess into loki_…

afd1ab6

…basic_workflow

nvaytet and others added 2 commits February 7, 2022 10:08

add reference for the article where data has been published

230c903

Apply automatic formatting

e5df6bc

nvaytet added 2 commits February 7, 2022 10:22

typo in author name

8f0b74f

Merge branch 'loki_basic_workflow' of github.com:scipp/ess into loki_…

f1d922e

…basic_workflow

nvaytet merged commit 637b96f into main Feb 7, 2022

nvaytet deleted the loki_basic_workflow branch February 7, 2022 09:31

		sample.masks[f"mask_{i}_xml"] = mask_xml["spectrum", :spectrum_size].data
		background.masks[f"mask_{i}_xml"] = mask_xml["spectrum", :spectrum_size].data

		import scippneutron as scn


		def load_isis(filename, spectrum_size, tof_bins):

		from .normalization import solid_angle, transmission_fraction


		def to_wavelength(

	Convertining and rebinning shifted data
	Convertining and rebinning shifted data

		background = (below.sum() + above.sum()) / sc.scalar(below.sizes[dim] +
		above.sizes[dim])

SANS Loki basic workflow #60

SANS Loki basic workflow #60

Conversation

wpotrzebowski commented Nov 2, 2021

SimonHeybrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvaytet left a 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

wpotrzebowski commented Nov 4, 2021

SimonHeybrock left a 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

SimonHeybrock left a 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

nvaytet commented Feb 4, 2022

wpotrzebowski commented Feb 4, 2022

wpotrzebowski commented Feb 5, 2022 • edited

nvaytet commented Feb 7, 2022 • edited

wpotrzebowski commented Feb 5, 2022 •

edited

nvaytet commented Feb 7, 2022 •

edited