Normalization step in the YMIR image workflow. #42

YooSunYoung · 2024-09-06T13:19:59Z

Normalization method is implemented.

~~Tests will come from different PR~~ Test included in this PR.

YooSunYoung · 2024-09-06T13:46:11Z

src/ess/imaging/normalize.py

+    """
+    sample_images = _select_images_by_key(image_stacks, ImageKey.SAMPLE)
+    dark_current = _select_image_by_key(bg_images, ImageKey.DARK_CURRENT)
+    return CleansedSampleImages(sample_images - dark_current)


This part is very slow... something must be wrong here.

YooSunYoung · 2024-09-06T13:54:29Z

src/ess/imaging/normalize.py

+def calculate_d0(background: BackgroundImage) -> D0:
+    """Calculate the D0 value from background image stack.
+
+    :math:`D0 = mean(background counts of all pixels)`
+    """
+    return D0(sc.mean(background))
+
+
+def calculate_d(samples: CleansedSampleImages) -> D:
+    """Calculate the D value from the sample image stack.


This part is so slow too.

docs/user-guide/histogram_mode_detector.ipynb

nvaytet · 2024-09-19T06:57:14Z

src/ess/imaging/normalize.py

+        OpenBeam = mean(open_beam, 'time')
+
+    """
+    return OpenBeamImage(sc.mean(open_beam, dim=TIME_COORD_NAME))


Question: Is the time coordinate here time during the measurement, or is it time-of-flight?

it's timestamp of each image, when it was taken.
It's an optical 2-d detector so it doesn't have time-of-flight.

nvaytet · 2024-09-19T07:03:30Z

src/ess/imaging/normalize.py

+        Threshold for the background pixel values.
+        Any pixel values less than ``background_threshold``
+        are replaced with ``background_threshold``.
+        Default is 1.0[counts].


Is this done to avoid nans when doing the division in the normalization?

Yes. There is another python project called pymureh and I mostly followed their method,
but I thought it was a bit weird to hard-code the number so I exposed it as an argument.

nvaytet · 2024-09-19T07:07:43Z

src/ess/imaging/normalize.py

+    .. math::
+
+        Background = mean(OpenBeam, 'time') - mean(DarkCurrent, 'time')
+


Can you also add (in words or in the math) the fact that you are applying a threshold after the operation?

nvaytet · 2024-09-19T07:08:25Z

src/ess/imaging/normalize.py

+
+    .. math::
+
+        CleansedSample_{i} = Sample_{i} - mean(DarkCurrent, dim='time')


Same here: also add (in words or in the math) the fact that you are applying a threshold after the operation?

nvaytet · 2024-09-19T07:11:20Z

src/ess/imaging/normalize.py

+    and returned negative values.
+    """
+    return AverageSamplePixelCounts(
+        _mean_all_dims(sample_images.data) - dark_current.data.mean()


I am wondering if sums should be used instead of means for both the AverageBackgroundPixelCounts and the AverageSamplePixelCounts?

Usually when we normalize by e.g. a monitor, we sum the counts?

But we don't take same number of dark-current/open-beam images as the sample images.

Probably best to discuss in person / over a call, as I am wondering if the reduction over the time dimension should also be sums instead of means?

If I understand correctly, it's to make sure everything has been scaled to the same integration time, before performing the division, right?

If I understand correctly, it's to make sure everything has been scaled to the same integration time, before performing the division, right?

Yes, that was the idea.

Or maybe it should be more like:

(sample_image.sum('time') / sample_image_integration_time - dark_frame.sum('time') / dark_frame_integration_time) / (openbeam_image.sum('time') / openbeam_image_integration_time - dark_frame.sum('time') / dark_frame_integration_time)

?

Number of images is same as the integration_time since the shutter(?) speed is fixed.

Ah so you're saying that doing a mean is essentially doing image.sum(time) / integration_time since you divide by the number of images (there is just a factor of time_shutter_is_open which cancels out everywhere)?

I guess if the shutter speed is adjustable this should be verified?

Are the dark frames and open beam always taken just before the sample is, or is a dark frame sometimes taken weeks in advance and used for several experiments in a row?

If it's the former, then I guess we are fine with means.

Ah so you're saying that doing a mean is essentially doing image.sum(time) / integration_time since you divide by the number of images (there is just a factor of time_shutter_is_open which cancels out everywhere)?

Yes...! That's what I meant.
But maybe I should check with Robin once more and mention this in the docstring.

I guess if the shutter speed is adjustable this should be verified?

Yes. Totally.

Are the dark frames and open beam always taken just before the sample is, or is a dark frame sometimes taken weeks in advanced and used for several experiments in a row?

They will be taken again every time just before or after the sample images.

Background images(dark-current and open-beam) takes much less time than samples and
Odin team wanted every types of images in one file.

If it's the former, then I guess we are fine with means.

Yes. In either case, I'll update the docstring.

I double checked with Søren and opened an issue as a reminder.

#48

So the exposure time is constant within a single file, but may vary in different files.

And it gives warning in the average/normalization functions about exposure time.

I chose warning over docstring because I think we should use exposure-time as you said,
and it should be avilable in the file.

nvaytet · 2024-09-19T14:18:43Z

src/ess/imaging/normalize.py

+def calculate_white_beam_background(
+    open_beam: OpenBeamImage,
+    dark_current: DarkCurrentImage,
+    background_threshold: BackgroundPixelThreshold = DEFAULT_BACKGROUND_THRESHOLD,


I'm thinking that BackgroundPixelThreshold should not have a default value here, but instead a default param should be set on the workflow/pipeline ?

It does have a default param in the workflow/pipeline.

The default value in the function signature is not used by the sciline.Pipeline.

I thought it'll make it easier to use the provider itself, i.e. for testing...

I would say if you want to use it for testing, then it's good that the parameter is required, so that you understand exactly what is being done?

An alternative would be to split it up into 2 providers: one that does the subtraction, and one that applies the threshold afterwards, with a very obvious name like ReplaceZerosAndNegativesWithOne?

I would say if you want to use it for testing, then it's good that the parameter is required, so that you understand exactly what is being done?

You can still test it with the default argument value though.

An alternative would be to split it up into 2 providers: one that does the subtraction, and one that applies the threshold afterwards, with a very obvious name like ReplaceZerosAndNegativesWithOne?

I didn't want to hard-code the numbers so the name should be different but
I can split this up into two steps if that makes it more interpretable.

Second thoughts... I think rounding up should be done within the normalization step since it's actually where it's needed.

@nvaytet I ended up inserting threshold applying steps instead of merging them into the normalization step
since they are used for calculating scale factor as well...

But I still don't see why we can't use the default argument value though

I'm saying that because I think in esssans, we don't have default arguments in providers, only default parameters on the workflow (unless I missed something somewhere?).
So I thought we should try to be consistent across techniques.

@neil Okay I fixed it accordingly...!

src/ess/imaging/normalize.py

nvaytet · 2024-09-19T14:20:00Z

src/ess/imaging/normalize.py

+def cleanse_sample_images(
+    sample_images: SampleImageStacks,
+    dark_current: DarkCurrentImage,
+    sample_threshold: SamplePixelThreshold = DEFAULT_SAMPLE_THRESHOLD,


Same as above: set the parameter on the workflow?

src/ess/imaging/normalize.py

nvaytet · 2024-09-19T14:22:17Z

src/ess/imaging/workflow.py

+            SamplePixelThreshold: DEFAULT_SAMPLE_THRESHOLD,
+            FileLock: DEFAULT_FILE_LOCK,
        },
    )


Tests will come from different PR

Unless this change is urgent (e.g. for the STAP demo), I would rather see the unit tests as part of the same PR?

Yeah I wanted to release it before Christian leaves for vacation.
But I think I can just test them by myself.... I'll add tests here.

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

tests/image_normalize_test.py

nvaytet · 2024-09-24T13:16:22Z

tests/image_normalize_test.py

+        assert isinstance(normalized, sc.DataArray)
+        assert normalized.sizes['time'] == 2
+        assert normalized.unit == "dimensionless"
+        assert_allclose(normalized, expected_normalized_sample_images)


Can you also add some tests on the functions that apply the thresholds on the sample and dark images?

@nvaytet Done...!

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

YooSunYoung commented Sep 6, 2024

View reviewed changes

YooSunYoung force-pushed the ymir-imaging branch from e228af4 to 6f4bc9f Compare September 10, 2024 07:35

YooSunYoung force-pushed the normalization branch 2 times, most recently from 84922f8 to dab573f Compare September 13, 2024 13:04

Base automatically changed from ymir-imaging to main September 16, 2024 11:33

YooSunYoung force-pushed the normalization branch 2 times, most recently from ca2c9ac to b20ae4f Compare September 16, 2024 12:50

YooSunYoung commented Sep 16, 2024

View reviewed changes

docs/user-guide/histogram_mode_detector.ipynb Outdated Show resolved Hide resolved

YooSunYoung changed the base branch from main to test-data September 18, 2024 07:39

YooSunYoung marked this pull request as ready for review September 18, 2024 07:40

YooSunYoung force-pushed the test-data branch from 35c3f41 to 9fb1e48 Compare September 18, 2024 07:42

YooSunYoung mentioned this pull request Sep 18, 2024

Update test dataset to include more sample images with better resolut… #45

Merged

YooSunYoung force-pushed the normalization branch from a11c081 to ffabecb Compare September 18, 2024 08:19

nvaytet self-assigned this Sep 19, 2024

nvaytet reviewed Sep 19, 2024

View reviewed changes

YooSunYoung marked this pull request as draft September 19, 2024 15:19

YooSunYoung marked this pull request as ready for review September 20, 2024 08:50

Base automatically changed from test-data to main September 20, 2024 11:12

YooSunYoung force-pushed the normalization branch from e2f173d to 1c1edb7 Compare September 20, 2024 11:17

YooSunYoung added 9 commits September 20, 2024 14:01

Add normalization step in the YMIR image workflow.

984bd2b

Load/separate image stacks by keys, without binning/grouping.

e342845

Add explanation why the image keys are treated like this.

f65a553

Update normalization workflow.

1f1924c

Wrap operation to be faster.

2dec9c8

Improve performance.

7516547

Add safety layer.

0bb208e

Replace negative value to positive value.

51f87cf

Add documentation.

e7cad1e

YooSunYoung and others added 18 commits September 20, 2024 14:01

Add default value for the background pixel threshold.

d72dfee

Turn negative pixel to 0.

4f8b4d1

Update workflow user guide.

2378d90

Add locking flag as a parameter.

bd03923

Update argument name to match other functions.

fe5b828

Update default file lock value.

ca14e1e

Add explanation about threshold.

d1d860a

Add file lock argument in rotation log.

16be580

Add user warning about exposure time.

ba4a16a

Fix typo

585a3e7

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

Fix typo [skip ci]

7abb529

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

Fix typo

3356ced

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

Rename dfactor to scale_factor

fe5754d

Move warning after error throwing line.

212bfa2

Fix average calculation.

60ac494

Add normalization workflow tests.

bc9af86

Insert threshold applying steps in the workflow and rename types.

469b5ea

Apply changes in the documentation.

3f5b477

YooSunYoung force-pushed the normalization branch from 1c1edb7 to 3f5b477 Compare September 20, 2024 12:01

Fix docstrings and function signatures.

d9e6b48

nvaytet reviewed Sep 24, 2024

View reviewed changes

YooSunYoung and others added 2 commits September 25, 2024 11:20

Fix typo [skip ci]

d7ad2b8

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

Add tests for threshold applying step.

58a36e0

nvaytet approved these changes Sep 25, 2024

View reviewed changes

YooSunYoung merged commit 92818cd into main Sep 25, 2024

YooSunYoung deleted the normalization branch September 25, 2024 13:41

		.. math::

		Background = mean(OpenBeam, 'time') - mean(DarkCurrent, 'time')


		.. math::

		CleansedSample_{i} = Sample_{i} - mean(DarkCurrent, dim='time')

Normalization step in the YMIR image workflow. #42

Normalization step in the YMIR image workflow. #42

Uh oh!

Conversation

YooSunYoung commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

nvaytet Sep 19, 2024 • 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!

nvaytet Sep 19, 2024 • 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!

YooSunYoung Sep 19, 2024 • 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!

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 Sep 20, 2024 • 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YooSunYoung commented Sep 6, 2024 •

edited

Loading

nvaytet Sep 19, 2024 •

edited

Loading

nvaytet Sep 19, 2024 •

edited

Loading

YooSunYoung Sep 19, 2024 •

edited

Loading

nvaytet Sep 20, 2024 •

edited

Loading

YooSunYoung Sep 25, 2024 •

edited

Loading