Skip to content
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

Use pixel masks from XML file for Loki #96

Merged
merged 19 commits into from
Feb 26, 2024
Merged

Use pixel masks from XML file for Loki #96

merged 19 commits into from
Feb 26, 2024

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Feb 20, 2024

Replaces #91

We had an issue with masking when merging multiple runs in the Loki workflow,

For example, to mask the straws at the top and bottom of the panel, we summed the counts inside each straw, and compared that to the number of pixels in each straw. This means that all you need to do is merge the events from 2 runs, and you will have approximately double the events but the number of pixels stays the same.
So now, in some straws, double the events means you are above the threshold and you start to mask less straws. The more runs you merge, the less straws you mask.

Here, we load an XML file containing the detectors to mask, as is done in the Zoom workflow.

Other changes:

  • Add the same mechanism as for ISIS workflows where local files can be used instead of pooch files by configuring DataFolder
  • Only use single files as inputs to load_nexus (instead of a file list). If we want to merge runs, we now have to set up a parameter table (added a section in the loki notebook that shows how to do this).

Note that the changes in masks seem to result in slightly worse results at low Q:

Before:
Screenshot at 2024-02-21 16-55-35

After:
Screenshot at 2024-02-21 16-55-42

This may suggest that we need to mask more pixels close to the beam stop? But that would be for the instrument scientist to decide.

@nvaytet nvaytet marked this pull request as draft February 21, 2024 09:17
@@ -87,46 +106,94 @@ def _merge_runs(data_groups: Iterable[sc.DataGroup], entries: Iterable[str]):
return out


def merge_sample_runs(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to specialise the types here for SampleRun and BackgroundRun, instead of having a single function with ScatteringRunType, because if not there would be multiple providers for LoadedFileContents[RunType] as the to_file_contents below also matches.

I don't think it's pretty. We also need to manually insert the merge_sample_runs and merge_background_runs into the pipeline if we want to use a parameter table with multiple runs.

Is there a better way to achieve this?
I basically want to merge the runs if a parameter table is supplied, but do nothing otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ignore this problem for now, as we plan to have a more generic solution.

@nvaytet nvaytet marked this pull request as ready for review February 21, 2024 15:58
@@ -87,46 +106,94 @@ def _merge_runs(data_groups: Iterable[sc.DataGroup], entries: Iterable[str]):
return out


def merge_sample_runs(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ignore this problem for now, as we plan to have a more generic solution.

lowcounts_straw_mask: Optional[DetectorLowCountsStrawMask],
beam_stop_mask: Optional[DetectorBeamStopMask],
tube_edge_mask: Optional[DetectorTubeEdgeMask],
def apply_pixel_masks(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this Loki-specific?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is another slightly annoying thing I had to work around.
In isis, the masks are applied on RawDataWithComponentUserOffsets, while in loki, they are applied on RawData.

Can we do it differently? I am not sure we want to introduce the ComponentUserOffsets everywhere?
I could make the apply_pixel_masks a common function and then have wrappers inside isis and loki that map the correct types, but I feel that would not be ideal either?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about Skadi etc.? My question was not necessarily if it can be the same for the ISIS instruments, more about all ESS instruments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will move into a common place for loki and leave the isis one in the submodule 👍

@nvaytet nvaytet merged commit a560960 into main Feb 26, 2024
3 checks passed
@nvaytet nvaytet deleted the loki-pixel-masks branch February 26, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants