Merge sans2d and zoom code into isis module #77

nvaytet · 2024-02-09T19:28:46Z

This unifies the sans2d and zoom code into the isis module.

I also used mantidio to make new files for sans2d. In the process, I also dropped the dummy tof coordinate from the data which just had a single (not very useful) bin. So I also made new files for zoom.

Fixes #48 because the new files all have the gravity coordinate added.
Gravity is needed to know which direction is up/down to apply beam center corrections perpendicular to the beam

Edit: Converting to draft because there is a small difference in the results. We are now just masking the second detector panel, when before it was sliced out. Maybe it can make a small difference to the beam center finder?

…ded data

nvaytet · 2024-02-09T19:30:55Z

src/esssans/loki/general.py

@@ -51,6 +52,16 @@ def get_monitor_data(
    return RawMonitor[RunType, MonitorType](out)


+def calibrate_monitor_position(


I don't like the fact that I needed to add this. Should the loki.get_monitor_data just return CalibratedMonitor instead of RawMonitor?

Yes, if we know that the raw files contain calibrated positions then we should directly return that.

I guess it depends what we mean by "calibrated". Does it mean that the positions have been modified to be correct, or does it simply mean that the positions are correct?

If the former, then we can't really say that this would return CalibratedMonitor because the positions have not been modified, they are "raw".

I changed to return CalibratedMonitor directly.

SimonHeybrock · 2024-02-12T05:13:25Z

docs/examples/beam-center-finder.ipynb

@@ -246,8 +246,14 @@
    "params[WavelengthBins] = sc.linspace(\n",
    "    'wavelength', start=2.0, stop=16.0, num=141, unit='angstrom'\n",
    ")\n",
-    "params[FileList[TransmissionRun[SampleRun]]] = params[FileList[SampleRun]]\n",
-    "params[FileList[EmptyBeamRun]] = ['SANS2D00063091.hdf5']\n",
+    "params[isis.Filename[TransmissionRun[SampleRun]]] = params[isis.Filename[SampleRun]]\n",


See #46, it seems it was forgotten to apply the fix here?

SimonHeybrock · 2024-02-12T05:17:50Z

src/esssans/common.py

+        if dim not in da.coords:
+            da = da.bin({dim: 1})


Why do we have to bin twice? That can be costly.

Edit: I see now it was moved here from conversions.py. Still my question stands.

I think it was a very lazy way of getting bin bounds that encompassed all the events.
I now changed it to computing the limits by hand if needed.

SimonHeybrock · 2024-02-12T05:23:39Z

src/esssans/isis/io.py

-def to_path(filename: FilenameType, path: DataFolder) -> FilePath[FilenameType]:
-    return f'{path}/{filename}'
-
-


Why was this removed? This is needed for generic instrument-independent (and pooch-independent) setup. My thought was that this would actually move to the top level eventually, i.e., not remain in the isis module.

I put this back in when I made a common data.py file. See if that works.

SimonHeybrock · 2024-02-12T05:26:10Z

src/esssans/isis/io.py

+def load_run(filename: FilePath[Filename[RunType]]) -> LoadedFileContents[RunType]:
+    return LoadedFileContents[RunType](sc.io.load_hdf5(filename))
+
+
+def load_direct_beam(filename: FilePath[DirectBeamFilename]) -> DirectBeam:
+    return DirectBeam(sc.io.load_hdf5(filename))
+
+
+providers = (read_xml_detector_masking, load_run, load_direct_beam)


I don't feel these belong here, since they only work with some special files that we made for the docs. isis.io should remain clear of that, i.e., contain only functionality that works on "regular" files.

SimonHeybrock · 2024-02-12T05:32:14Z

src/esssans/isis/zoom.py

I would avoid naming this isis.zoom. It contains special data for docs/testing, not just a general workflow for Zoom. I think we should very clearly separate the two. Maybe isis.zoom_data?

Should I then just make 2 sub-folders for sans2d and zoom?

Cause I thought it would be a little strange to have a zoom_data.py, and a sans2d.py file that contains both things to do with (pooch) data and masking. So I wanted to make:

sans2d/data.py sans2d/masking.py zoom/data.py

I guess we could also just make

sans2d_data.py sans2d_masking.py zoom_data.py

but I thought the separation would be clearer with subfolders.

Alternatively, I could make a common data.py file for both zoom and sans2d, and then just have a sans2d.py file that contains masking providers (where future sans2d-specific providers could then be added).

Why would you have sans2d contain both things? We should treat the two in the same manner and split the files by purpose.

I thought masking from masks files is ISIS-specific, not instrument specific, so I would leave that as isis.masking?

SimonHeybrock · 2024-02-12T05:34:06Z

src/esssans/loki/general.py

@@ -51,6 +52,16 @@ def get_monitor_data(
    return RawMonitor[RunType, MonitorType](out)


+def calibrate_monitor_position(


Yes, if we know that the raw files contain calibrated positions then we should directly return that.

…notebook

SimonHeybrock · 2024-02-12T14:08:00Z

src/esssans/common.py

+                ),
+            )
+        else:
+            new_bins = np.union1d(edges.values, da.coords[dim].values)


What if the units do not match?

I think it's actually ok in this particular case, because below, we do

new_bins = sc.array(dims=[dim], values=new_bins, unit=edges.unit) out = da.bin({dim: new_bins})

So the bad unit would be caught by the da.bin (I think?).

However, it's not so obvious to someone reading the code.
I can add an explicit check if you like?

I think we should add a conversion (or check) of the mask edges to the correct unit close to the beginning of the function, where also other checks are performed. Otherwise we get a cryptic error from sc.bin here, or worse, someone will modify the code and the coincidental unit check will suddenly disappear.

nvaytet · 2024-02-12T15:57:45Z

So I've narrowed down the difference in results between the old implementation and new implementation for the SANS2D workflow to the values of the variances.

The data values on the final I(Q) are identical.

For the variances, if I use the default pipeline, I get larger variances (visible ay high Q) in the new implementation.

If I use the mode which drops the variances when broadcasting, the curves seem to overlap

I think it comes from the fact that the difference between the old and the new is that in the new, we are keeping both detector panels (and masking the second panel), while in the old we were slicing that panel out when loading the data.
I think if we are broadcasting variances, we are broadcasting to the number of pixels, not looking at the masks, so the variances could still end up being larger, even if we have masked the pixels?

Interestingly, looking directly at the variances of the final result (sc.variances(iofq)) shows that even with the UncertaintyBroadcastMode.drop mode, the variances in the new implementation are still larger that in the old one.

Is this because there are still some reduction operations that occur over all the pixels?

… is provided

SimonHeybrock · 2024-02-14T04:25:39Z

I think if we are broadcasting variances, we are broadcasting to the number of pixels, not looking at the masks, so the variances could still end up being larger, even if we have masked the pixels?

That may be it. We should probably take into account masks in the upper-bound estimate, i.e., exclude masked pixels from the scale factor? Can you try if you observe the same difference in event-mode?

Is this because there are still some reduction operations that occur over all the pixels?

Can't think of anything. The solid angle should ignore masking. Are you sure the same number of non-masked pixels is used? Is there some duplication?

nvaytet · 2024-02-14T16:03:10Z

It seems I can no longer reproduce the differences above if I set params[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop

I set up the params and then did

providers = (
    sans.providers + isis.providers + isis.data.providers + isis.sans2d.providers
)
providers = list(providers + (
    sans.transmission_from_background_run,
    sans.transmission_from_sample_run,
))

pipeline_new = sciline.Pipeline(providers=providers, params=params)

pipeline_old = sciline.Pipeline(providers=providers, params=params)
pipeline_old.insert(isis.general.get_detector_data_sliced)

where

def get_detector_data_sliced(
    dg: LoadedFileContents[RunType],
) -> RawData[RunType]:
    return RawData[RunType](dg['data']['spectrum', :61440].copy())

Then, when computing IofQ

t = BackgroundSubtractedIofQ
old = pipeline_old.compute(t)
new = pipeline_new.compute(t)

both results have identical variances.

I must have messed up in my notebook above.

nvaytet · 2024-02-14T16:03:42Z

Can you try if you observe the same difference in event-mode?

Results are the same in dense and event modes.

nvaytet · 2024-02-15T07:33:38Z

We should probably take into account masks in the upper-bound estimate, i.e., exclude masked pixels from the scale factor?

Should we do that in a separate PR?

SimonHeybrock · 2024-02-15T07:41:09Z

We should probably take into account masks in the upper-bound estimate, i.e., exclude masked pixels from the scale factor?

Should we do that in a separate PR?

Yes (see #89), but I am not sure what you said above. Is the issue resolved otherwise, or is there still an unknown source of difference?

nvaytet · 2024-02-15T07:43:27Z

Is the issue resolved otherwise, or is there still an unknown source of difference?

I will check one last time today, but I think there is no difference if we drop the variances. I think I must have messed up in my notebook when I first reported the issue. I will also check with a quick fix that if we take into account the masks in the broadcast of variances, it is also ok.

nvaytet · 2024-02-15T09:29:14Z

I found the bug in my notebook. I was checking sc.variances(new.hist() - old.hist()) instead of sc.variances(new.hist()) - sc.variances(old.hist()). The latter is zero everywhere.

nvaytet · 2024-02-15T09:42:37Z

I have now also verified that taking the masks into account when doing the variance broadcast yields the same results as before.

nvaytet added 18 commits February 9, 2024 15:32

fix wavelength masking function

8ab0169

add possibility of shifting monitor position

a5081f9

add general providers

48a657f

fix io providers and remove single-bin tof coordinate from mantid loa…

159b6e3

…ded data

add sans2d and zoom specific files

871f327

cleanup zoom

1a6b0a8

add dummy provider in loki for calibrated monitor

d25df8d

fix masking range test

7931b08

migrate sans2d tests

037ab8d

remove unused files

ce7d831

update notebooks

1f5e10e

fix direct beam provider in tests

930b6b3

fix phi coordinate in beam center finder

fb27e52

fix module path

df08d43

finish fixing sans2d tests

95829f9

update beam center finder notebook

bf95f6b

flake8

56b8545

formatting

db411ee

nvaytet commented Feb 9, 2024

View reviewed changes

nvaytet added 3 commits February 10, 2024 10:11

fix get_path call

145371b

fix sans notebook and make single get_detector_data

eba3fd7

remove debugging cells from notebook

2b77488

nvaytet marked this pull request as draft February 10, 2024 09:39

SimonHeybrock reviewed Feb 12, 2024

View reviewed changes

nvaytet added 6 commits February 12, 2024 12:08

bin only once and compute bounds manually if needed

60b332a

add sans.transmission_from_sample_run provider in beam center finder …

75193ed

…notebook

make subfolders for isis instruments

54cebda

make a common isis data registry

87e740a

cleanup and update notebooks

d133194

flake8

434595a

return CalibratedMonitor directly in loki

209bf13

SimonHeybrock reviewed Feb 12, 2024

View reviewed changes

nvaytet added 6 commits February 12, 2024 15:08

formatting

d043a0d

fix normalization and sans2d tests

93e0160

flake8

14f4092

Merge branch 'main' into merge-sans2d-zoom

c128209

add basic zoom tests

e440743

fix docs build

0cf1868

add test to make sure workflow can run without gravity if beam center…

6b116bc

… is provided

nvaytet marked this pull request as ready for review February 12, 2024 16:28

add unit conversion for edges in mask_range

90dc80b

SimonHeybrock approved these changes Feb 13, 2024

View reviewed changes

nvaytet marked this pull request as draft February 14, 2024 15:11

nvaytet marked this pull request as ready for review February 14, 2024 16:04

nvaytet merged commit 53caad4 into main Feb 15, 2024
3 checks passed

nvaytet deleted the merge-sans2d-zoom branch February 15, 2024 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge sans2d and zoom code into isis module #77

Merge sans2d and zoom code into isis module #77

nvaytet commented Feb 9, 2024 •

edited

Loading

nvaytet Feb 9, 2024

SimonHeybrock Feb 12, 2024

nvaytet Feb 12, 2024

nvaytet Feb 12, 2024

SimonHeybrock Feb 12, 2024

SimonHeybrock Feb 12, 2024

nvaytet Feb 12, 2024

SimonHeybrock Feb 12, 2024

nvaytet Feb 12, 2024

SimonHeybrock Feb 12, 2024

SimonHeybrock Feb 12, 2024

nvaytet Feb 12, 2024

SimonHeybrock Feb 12, 2024

nvaytet Feb 12, 2024

nvaytet Feb 12, 2024

SimonHeybrock Feb 12, 2024

SimonHeybrock Feb 12, 2024

SimonHeybrock Feb 12, 2024

SimonHeybrock Feb 12, 2024

nvaytet Feb 12, 2024 •

edited

Loading

SimonHeybrock Feb 13, 2024

nvaytet commented Feb 12, 2024

SimonHeybrock commented Feb 14, 2024

nvaytet commented Feb 14, 2024

nvaytet commented Feb 14, 2024

nvaytet commented Feb 15, 2024

SimonHeybrock commented Feb 15, 2024 •

edited

Loading

nvaytet commented Feb 15, 2024

nvaytet commented Feb 15, 2024

nvaytet commented Feb 15, 2024

		@@ -51,6 +52,16 @@ def get_monitor_data(
		return RawMonitor[RunType, MonitorType](out)


		def calibrate_monitor_position(

		def to_path(filename: FilenameType, path: DataFolder) -> FilePath[FilenameType]:
		return f'{path}/{filename}'

Merge sans2d and zoom code into isis module #77

Merge sans2d and zoom code into isis module #77

Conversation

nvaytet commented Feb 9, 2024 • edited Loading

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

nvaytet Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvaytet commented Feb 12, 2024

SimonHeybrock commented Feb 14, 2024

nvaytet commented Feb 14, 2024

nvaytet commented Feb 14, 2024

nvaytet commented Feb 15, 2024

SimonHeybrock commented Feb 15, 2024 • edited Loading

nvaytet commented Feb 15, 2024

nvaytet commented Feb 15, 2024

nvaytet commented Feb 15, 2024

nvaytet commented Feb 9, 2024 •

edited

Loading

nvaytet Feb 12, 2024 •

edited

Loading

SimonHeybrock commented Feb 15, 2024 •

edited

Loading