Use normalisation in detector/q space to account for pixel differences #97

arm61 · 2022-02-01T10:27:36Z

A first draft (please comment on the text cause I imagine the explanation isn't great) going through performing the normalisation on dimensions of 'detector_id' and 'Q' instead of just 'Q'.

jl-wynen

I think the explanations are understandable.

docs/instruments/amor/amor_reduction.ipynb

Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>

nvaytet · 2022-02-07T12:43:04Z

docs/instruments/amor/amor_reduction.ipynb

   "metadata": {},
   "source": [
-    "And we then convert to $Q$ as well"
+    "However, if we want to obtain the normalisation between the sample data and the reference (accounting for detector pixel discontinuities), we want to work with the data with dimensions of `'detector_id'` and `'Q'` (the `*_q_binned` objects). "


I think this is making reading the notebook read a bit more confusing (talking about the *q_binned quantities).
I would re-work the notebook so that this is what you want to do from the beginning.
You don't define any *_q_summed quantities, you just stick with the *_q_binned (and possibly give them a better name), just say that you're making a quick preview in Q by plotting the sum over detector_id, just as a sanity check.

And just move on to making the normalization as you want it.
There is no need to keep in the notebook the fact that we started by doing this, and then we improved it. The reader doesn't need to know about the developement history of the workflow.

This is a good point. I am planning to add the footprint correction soon so I suggest that when I add that we rework the whole thing (I suggest I get a first draft soon and you can go through it). This approach might also help to understand why mean instead of sum before (cause at the moment I am not 100 % sure why mean gives the correct result and sum doesn't).

I don't think it's too much work to change what I suggested, probably just a small number of cells that need changing?

(cause at the moment I am not 100 % sure why mean gives the correct result and sum doesn't)

maybe we should dig down a little bit more and understand 100% before we move forward, as we may forget at some point that we were not 100% sure, and after a while everyone just assumes it's all correct?

The order of the operations is a little different now, maybe if we write them all down we can see where the mean comes from?

Very much agree, and in the process of second drafting this I think I am grepping it a bit better

docs/instruments/amor/amor_reduction.ipynb

…the notebook has had a large rewrite

arm61 · 2022-02-08T15:15:00Z

I have added some new features (specifically the footprint correction and the supermirror calibration which were present in the original iteration of this code. Additionally, there has been a lot of new writing in the notebook. Feedback pls.

I will add tests in the coming days.

Fix the Note box formatting overflow

arm61 · 2022-02-10T15:30:47Z

Have just spoken with the Amor instrument scientist and he recommends a slightly different footprint correction that I will look at tomorrow morning.

Use <div> instead of <a> tag for citation links to work in sphinx

arm61 · 2022-03-01T10:56:38Z

Rebased to main now that there is a 0.4 release.

nvaytet · 2022-03-07T10:11:45Z

Not sure what happened in the rebase, but I am seeing in the diff the changes from the release that are unrelated to this PR...

arm61 · 2022-03-07T10:22:40Z

Bugger, suggestions to resolve (squash?)

nvaytet · 2022-03-07T10:34:13Z

How did you do your rebase? You could try merging instead?

arm61 · 2022-03-07T10:42:01Z

Merge main?

arm61 · 2022-03-07T10:42:20Z

How did you do your rebase?

Honestly can't remember now...

nvaytet · 2022-03-11T10:10:49Z

docs/instruments/amor/amor_reduction.ipynb

-    "reference_wav = reference.transform_coords([\"wavelength\"], graph=graph)\n",
-    "reference_wav = sc.bin(reference_wav, edges=[wavelength_edges])\n",
-    "reference_wav.bins.concatenate('detector_id').plot()"
+    "fwhm_to_std = sc.scalar(2.) * sc.sqrt(sc.scalar(2.) * sc.log(sc.scalar(2.)))\n",


I seem to remember these were in a helper function before. Maybe we should do this again?

I have added it as a constant in the amor.tools directory for now. Let me know if you think a function is more valuable.

src/ess/amor/calibrations.py

nvaytet · 2022-03-16T11:43:07Z

docs/instruments/amor/amor_reduction.ipynb

+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "from ess.amor.tools import FWHM_TO_STD\n",


You could also consider making a function that takes in the sample_size / beam_on_sample and then converts to std by multiplying by fwhm_to_std.

e.g.

def fwhm_to_std(x): return x * sc.scalar(2.) * sc.sqrt(sc.scalar(2.) * sc.log(sc.scalar(2.)))

?

Note that you also need to replace fwhm_to_std = sc.scalar(2.) * sc.sqrt(sc.scalar(2.) * sc.log(sc.scalar(2.))) further down below

My rational for the current implementation was that this allows for the inverse to give the equivalent of STD_TO_FWHM without another function.

nvaytet · 2022-03-16T11:44:41Z

docs/instruments/amor/amor_reduction.ipynb

+   "source": [
+    "normalized = sample_norm / reference_norm\n",
+    "normalized.coords['footprint_ratio'] = ratio\n",
+    "sc.plot(normalized)"


Just remove the plot altogether?

docs/instruments/amor/amor_reduction.ipynb

nvaytet · 2022-03-16T12:06:47Z

docs/instruments/amor/amor_reduction.ipynb

+   "id": "64a82569-5c12-480e-a120-85b0215f1e31",
+   "metadata": {},
+   "source": [
+    "Finally, we perfrom a mean along the `'detector_id'` to obtain the normalised reflectometry profile.\n",


And are you sure about the mean here? because the values at low q in the final normalized plot went from ~0.1 to ~10

If you had 3 detector pixels, before we were doing

(counts_1 + counts_2 + counts_3) / (reference_1 + reference_2 + reference_3)

and now we are doing

(counts_1/reference_1 + counts_2/reference_2 + counts_3/reference_3) / 3

?

We should check the final values against Jochen's code.

Because we have masked pixels on reference I think that the second is the one we want...There is also the fact that using sum gives a result at looks wrong (i.e. the critical edge should be flat). Jochen's code doesn't do the normalisation, I need to go a level up to eos.py which can do the normalisation but will take quite a while to get to work.

To me, a mean doesn't really make sense, but maybe that's just me. We need to be careful about what we mean by zero counts. Is it just zero because we didn't run the experiment long enough, and statistically we would have had some counts if we recorded longer? Or does it mean zero in the sense that there simply cannot be counts there, just like if you did not have a detector pixel there?

I don't think just saying the shape looks better is enough justification, especially since the values have changed by so much. We need to understand what is correct.
The fact that there are nan values would mess up both a sum and and mean operation so that should not be a reason to use one or the other?

I very much agreed looks right isn't good scientific justification and we still need to understand it (but it indicates it is correct). Changed by so much compared to?

The sum counts all the pixels and does no division, the mean counts the pixels that are un-masked and divides by the number of un-masked pixels. The number of unmasked pixels changes as a function of q, which is why there is a difference between our sum and our mean. And I think it makes sense to consider that weighting (with sum doesn't do).

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

…verting to and from fwhm

docs/instruments/amor/amor_reduction.ipynb

src/ess/amor/tools.py

nvaytet

Had another look and changes look ok to me.

arm61 · 2022-03-29T14:16:39Z

Same here. Happy for you to merge whenever.

Use normalisation in detector/q space to account for pixel differences

bf1832d

arm61 requested review from nvaytet and jl-wynen February 1, 2022 10:27

Apply automatic formatting

6781379

jl-wynen reviewed Feb 1, 2022

View reviewed changes

docs/instruments/amor/amor_reduction.ipynb Outdated Show resolved Hide resolved

docs/instruments/amor/amor_reduction.ipynb Outdated Show resolved Hide resolved

arm61 and others added 3 commits February 1, 2022 13:11

Update docs/instruments/amor/amor_reduction.ipynb

28fd9f1

Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>

Mention inf as well as nan

5cdc027

Merge

c219db9

nvaytet reviewed Feb 7, 2022

View reviewed changes

Add footprint correction and super mirror calibration. Additionally, …

377c3a0

…the notebook has had a large rewrite

arm61 and others added 6 commits February 8, 2022 16:15

Merge branch 'main' into normalisation

b3377c9

flake8

2876432

Merge branch 'normalisation' of github.com:scipp/ess into normalisation

405c2be

Apply automatic formatting

719c7d3

close the div

3104687

Merge pull request #103 from scipp/fix_note_box_formatting

68c0119

Fix the Note box formatting overflow

arm61 marked this pull request as draft February 10, 2022 15:30

arm61 and others added 11 commits February 11, 2022 11:20

Fix reference duplication

9ceb5e1

Merge

6e1c904

use <div> instead of <a> tag for citation links to work in sphinx

490ee4e

Move to using div

60ac83c

Remove .DS store

00b7757

Apply automatic formatting

75f57f2

Merge pull request #104 from scipp/div_for_citation_links

3feebf5

Use <div> instead of <a> tag for citation links to work in sphinx

add style guide for contributions

63e29b7

rework documentation section

3a1f7da

add section on code formatting

209ed3f

add link to ess logging and change folder to docs

c2eab12

arm61 and others added 3 commits March 1, 2022 11:55

ticking some more of Neils comments off

d7e8ba0

Apply automatic formatting

518aeb9

Merge branch 'normalisation' of github.com:scipp/ess into normalisation

a31ceb3

Trying to get changes to be respected

600728c

nvaytet reviewed Mar 11, 2022

View reviewed changes

arm61 and others added 6 commits March 15, 2022 16:14

Add FWHM_TO_STD

c4f7088

Apply automatic formatting

4e7ce46

Use suggstion from Simon for calibration

537cf90

Metadata removed

4a9dd2c

Merge branch 'normalisation' of github.com:scipp/ess into normalisation

b8247bd

Apply automatic formatting

6ecf2d3

nvaytet reviewed Mar 16, 2022

View reviewed changes

arm61 and others added 3 commits March 16, 2022 14:15

Update docs/instruments/amor/amor_reduction.ipynb

910c09d

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

Improve the wording to explain the main and add the functions for con…

5b12203

…verting to and from fwhm

Merge

804d9d0

nvaytet reviewed Mar 18, 2022

View reviewed changes

docs/instruments/amor/amor_reduction.ipynb Outdated Show resolved Hide resolved

src/ess/amor/tools.py Outdated Show resolved Hide resolved

src/ess/amor/tools.py Outdated Show resolved Hide resolved

arm61 and others added 3 commits March 18, 2022 15:16

Comments from Neil

f4cb4d0

Remove whitespace

cc5bd8e

Apply automatic formatting

e2a88c8

nvaytet approved these changes Mar 29, 2022

View reviewed changes

nvaytet merged commit 07dc1ca into main Mar 29, 2022

nvaytet deleted the normalisation branch March 29, 2022 14:39

arm61 mentioned this pull request Apr 11, 2022

Prepare v0.5.0 #117

Merged

Use normalisation in detector/q space to account for pixel differences #97

Use normalisation in detector/q space to account for pixel differences #97

Uh oh!

Conversation

arm61 commented Feb 1, 2022

Uh oh!

jl-wynen left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

arm61 commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arm61 commented Feb 10, 2022

Uh oh!

arm61 commented Mar 1, 2022

Uh oh!

nvaytet commented Mar 7, 2022

Uh oh!

arm61 commented Mar 7, 2022

Uh oh!

nvaytet commented Mar 7, 2022

Uh oh!

arm61 commented Mar 7, 2022

Uh oh!

arm61 commented Mar 7, 2022

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!

Uh oh!

nvaytet Mar 16, 2022 • 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!

arm61 Mar 17, 2022 • 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!

arm61 Mar 17, 2022 • 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!

Uh oh!

Uh oh!

Uh oh!

nvaytet left a comment

Choose a reason for hiding this comment

Uh oh!

arm61 commented Mar 29, 2022

Uh oh!

Uh oh!

arm61 commented Feb 8, 2022 •

edited

Loading

nvaytet Mar 16, 2022 •

edited

Loading

arm61 Mar 17, 2022 •

edited

Loading

arm61 Mar 17, 2022 •

edited

Loading