Conversation
github-actions
Bot
changed the title
| "source": [ | ||
| "freia_mcstas = freia.FreiaMcStasWorkflow(wavelength_from='analytical')\n", | ||
| "freia_mcstas[Filename[SampleRun]] = '../../../265305.h5'\n", | ||
| "freia_mcstas[KeepEventTimeOffset] = True\n", |
There was a problem hiding this comment.
I'll upload these files to the pooch repo
| "source": [ | ||
| "workflow = FreiaMcStasWorkflow(run_norm=RunNormalization.monitor_histogram)\n", | ||
| "workflow[Filename[SampleRun]] = '../../../265305.h5'\n", | ||
| "workflow[Filename[ReferenceRun]] = '../../../265301.h5'\n", |
There was a problem hiding this comment.
I'll upload these files or similar to the pooch repo.
| : | ||
| Qx-values. | ||
| Approximate the flight time using the straight sample-to-detector distance, | ||
| as in ScippNeutron's gravity correction. |
There was a problem hiding this comment.
ScippNeutron's gravity correction.
Is this something that is already implemented in scippneutron or is it just an example copied from the docs?
If the former, can we import from scippneutron and use here instead of re-implementing?
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "event_wavelengths = unwrapped.bins.constituents['data'].coords['wavelength']\n", |
There was a problem hiding this comment.
Can we do this differently than having to go via .bins.constituents?
Am I right in understanding that you want to remove the wavelengths that have been assigned NaN?
Can we instead do something like unwrapped.hist(wavelength=1).sum() and unwrapped.sum() to get the N events out of M?
Histogramming into 1 bin should discard all the NaNs.
| "pp.plot(wavelength_image, norm='log', title='Wavelength across the detector', vmin=1e0)" | ||
| ] | ||
| } | ||
| ], |
There was a problem hiding this comment.
Is it worth adding at the end of the notebook an inspector plot, where we histogram in longitude, height and wavelength? And then draw some rectangles around some regions?
| "\n", | ||
| "Compute one reflectivity curve for each incident beam using a sample measurement and a direct-beam measurement taken without a sample. Normalize by an incident wavelength monitor, select matching peaks, and compare the three curves with the silicon reflectivity reference.\n", | ||
| "\n", | ||
| "The sample and direct-beam runs must have matching slit and chopper settings. This example uses local files with WFM chopper settings." |
There was a problem hiding this comment.
This example uses local files with WFM chopper settings.
Not sure what "WFM chopper settings mean". Does it mean we use results from runs that were recorded in WFM mode? or data was recorded while WFM choppers were in use?
There was a problem hiding this comment.
I don't understand the distinction? The Freia instrument has more or less 2 chopper modes. One high resolution mode where the WFM choppers are in use, and one where they are not in use. The McStas model can simulate both modes. In this case the data comes from a simulation where the WFM choppers are active.
Does that answer the question?
There was a problem hiding this comment.
Hmm it was badly formulated from my part. I think I just repeated myself twice (probably I was editing my comment and it went wrong).
What I meant to say is that "WFM chopper settings" is unclear to the reader I think. In addition, I think we don't care that the files are 'local'?
Instead we should write something like "we use results recorded from runs where the WFM choppers were in use".
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "%matplotlib inline\n", |
| """Corrects the data by the size of the footprint on the sample.""" | ||
| return da / sc.sin(da.coords['theta']) | ||
|
|
||
| def prepare_sample( |
There was a problem hiding this comment.
'prepare' is a little vague. Can we use a name which is more descriptive?
| low, high = low.to(unit=coord.unit), high.to(unit=coord.unit) | ||
| if not (low <= high).value: | ||
| raise ValueError(f'Reversed ROI bounds for {name!r}.') | ||
| (event_masks if is_event_coord else masks)[f'roi_{name}'] = ~( |
There was a problem hiding this comment.
So here, if you have data which is binned in e.g. 'x' and 'y', 'x' and 'y' will be found both in the da.coords and in the da.bins.coords. If I followed correctly, we are first looking into the .bins.coords and if it's there, putting a mask on each event.
However, in that case it would be much cheaper to mask the bins rather than the events.
Should we first check if the key is found in the da.coords?
| ) | ||
|
|
||
|
|
||
| def _detector_geometry(data, detector_name) -> sc.DataArray: |
There was a problem hiding this comment.
The best thing here would have been to put the mcstas events inside a coda skeleton for Freia. Then we would avoid a lot of this code...
| normal = direct_beam.coords['sample_surface_normal'] | ||
| normal = normal / sc.norm(normal) | ||
| outgoing = direct_beam.bins.coords['outgoing_direction'] | ||
| reflected = outgoing - 2 * sc.dot(outgoing, normal) * normal |
There was a problem hiding this comment.
Can you explain or add a link as to where these formulas come from?
There was a problem hiding this comment.
Mirroring the outgoing direct beam in the sample plane produces the outgoing beam if it had been reflected. That tells us what (angle of reflection) theta bin and correspondingly what Q bin it should be assigned to when we compare the direct beam intensity to the reflected intensity.
There was a problem hiding this comment.
The explanations should go close to the code (either comment or docstring)
…th-lookup-table.ipynb Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
|
I think a lot of the comments here are good, but they are not critical. Can I collect them to a separate issue and finish that up later? Then we can merge and deploy this and we have something to show for Tuesday. |
2 participants
No description provided.