-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Zoom data reduction #50
Conversation
@@ -0,0 +1,43 @@ | |||
# SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the sans2d
submodule be part of isis
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, but I want to keep any unrelated changes out of this PR. We can perform such restructuring and rollout of changes in a follow-up.
" sans.isis.transmission_from_background_run,\n", | ||
" sans.isis.transmission_from_sample_run,\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are those not part of the isis.providers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because someone might want to set a separate transmission run, and in that case this should not be added.
docs/examples/zoom.ipynb
Outdated
"metadata": {}, | ||
"source": [ | ||
"If Mantid is available, we can use it to load data files.\n", | ||
"**You must configure the `DataFolder` below to point to the directory containing the data files.**\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"da = iofq.compute()\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/esssans/isis/data.py
Outdated
@@ -0,0 +1,66 @@ | |||
# SPDX-License-Identifier: BSD-3-Clause | |||
# Copyright (c) 2023 Scipp contributors (https://github.com/scipp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright (c) 2023 Scipp contributors (https://github.com/scipp) | |
# Copyright (c) 2024 Scipp contributors (https://github.com/scipp) |
'andru_test.xml': 'md5:c59e0c4a80640a387df7beca4857e66f', | ||
'left_beg_18_2.xml': 'md5:5b24a8954d4d8a291f59f5392cd61681', | ||
'right_beg_18_2.xml': 'md5:fae95a5056e5f5ba4996c8dff83ec109', | ||
'small_bs_232.xml': 'md5:6d67dea9208193c9f0753ffcbb50ed83', | ||
'small_BS_31032023.xml': 'md5:3c644e8c75105809ab521773f9c0c85b', | ||
'tube_1120_bottom.xml': 'md5:fe577bf73c16bf5ac909516fa67360e9', | ||
'tubes_beg_18_2.xml': 'md5:2debde8d82c383cc3d592ea000552300', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what the different masks represent? (i.e. what they are trying to mask?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in detail. Some tubes, beamstop, sample holder, but there appear to be multiple varieties. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking since we started adding short comments describing what the different files are, we could add some descriptions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extended the description.
src/esssans/isis/io.py
Outdated
DataFolder = NewType('DataFolder', str) | ||
|
||
|
||
class Path(sciline.Scope[FilenameType, str], str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path
will very commonly refer to pathlib.Path
. Can we avoid using the same name (especially as we sometimes from types import *
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to FilePath
.
especially as we sometimes
from types import *
?
I want to get away from that. As you can see I have kept providers and types more structured here, no placing anything in a "meaningless" types.py
. Still not there yet, but I hope it will work nice eventually.
src/esssans/isis/io.py
Outdated
return f'{path}/{filename}' | ||
|
||
|
||
def read_xml_detector_masking(filename: Path[PixelMaskFilename]) -> MaskedDetectorIDs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the format of those xml mask files specific to Mantid or are they also used by other software? If the former, should this move into mantidio
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, XML is used elsewhere, but the files are afaik pecific to Mantid.
data_ws = loaded | ||
else: | ||
# Separate data and monitor workspaces | ||
data_ws = loaded.OutputWorkspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we get hold of the monitor data in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mantid stores a link to the monitor workspace in the data workspace and that is where ScippNeutron pulls the monitors from, i.e., the fact that it returns both is redundant.
src/esssans/isis/visualization.py
Outdated
import scipp as sc | ||
|
||
|
||
def plot_instrument(da: sc.DataArray, pixels_per_tube: int = 512, **kwargs: Any) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you thinking this is generic enough that it can be used for other isis instruments? I guess it would work for sans2d as well. But then it should also work for other flat panels, not just isis panels?
Should we call it something else that implies it is plotting a flat panel? As it wouldn't work for something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any ISIS SANS instruments that do not have tubes that we have to worry about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it.
Parameters | ||
---------- | ||
da: | ||
The data array to plot. Must have a 'position' coord and a single dimension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to flatten if there are more than one dimension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but this is for ISIS data, which has been loaded with Mantid, so why complicate things? It can be extended once/if required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered doing this, but then it would complicate since we would need to maybe support summing some dims but flattening others, I didn't think it was worth it at this point. After all, this is not an ESS instrument.
folded.coords['y'] = y.min('x') | ||
else: | ||
raise ValueError( | ||
'Cannot plot 2-D instrument view of data array with non-constant ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really have to raise or would it just make a ragged detector panel, like the other plots we have with 2d coords?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah maybe it's because the x coord is already 2d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Scipp (and plotting, I guess?) do not support multiple 2-D coords. Or is Plopp supporting this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is Plopp supporting this?
No, we are not.
src/esssans/isis/visualization.py
Outdated
'Cannot plot 2-D instrument view of data array with non-constant ' | ||
'y coordinate along tubes. Use scippneutron.instrument_view instead.' | ||
) | ||
plot_kwargs = dict(aspect='equal', norm='log', figsize=(6, 10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the aspect='equal'
make sense for this, but I find it a little strange that it has log
as a default, and also changes the figsize? This size would not work e.g. for the sans2d panel.
There are still some position offsets to handle (from user file), but since I do not know when we will get the information on what to implement I will merge and handle the remaining bits in a follow-up. |
This adds a data reduction workflow for Zoom. The main addition is handling of file I/O and reading masks from a series of XML files.
esssans.isis.mantidio
provides I/O wrapper depending on Mantid.get_path
stuff from the docs notebooks, but adding a provider that converts filenames into paths. This can either be configured to use a configured data folder, or pooch, or anything else someone might come up with (such as search known locations, ...).