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

Reduction process: effective-instrument geometry. #461

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

ekapadi
Copy link
Contributor

@ekapadi ekapadi commented Sep 26, 2024

Description of work

At the end of the reduction process, the instrument associated with each output workspace is modified. A new effective instrument is substituted for the instrument attached to each workspace. This instrument has the same number of pixels as there are group-ids, and the location of each pixel is set to the mean location of the unmasked original pixels participating in that pixel group. By implication, this substitution results in there being one pixel per spectrum in each output workspace. (An exception to this substitution is the combined pixel-mask workspace, which is not modified.)

Explanation of work

This commit includes the following changes:

  • A new EffectiveInstrumentRecipe implemented as a subrecipe called by ReductionRecipe for each grouping;

  • Modification of ReductionIngredients to include the unmasked PixelGroups;

  • Modification of SousChef.prepReductionIngredients to include the unmasked PixelGroups;

  • Modification of existing unit tests, and implementation of new unit tests to verify the new subrecipe's execution.

A known issue: EditInstrumentGeometry deletes, but does not regenerate the XML associated with the newly-edited instrument. Unfortunately, this tends to generate a lot of misleading log messages when the associated workspaces are saved, and then reloaded. This isn't so much an issue with EditInstrumentGeometry specifically, but rather with the Mantid instrument framework.

To test

Existing unit tests have been modified, and new unit tests have been added to cover the new subrecipe. A new CIS-test script has been added at tests/cis_tests/effective_instrument_script.py.

Dev testing

In addition to running the unit tests, and the CIS-test script, the GUI-panels should be run for the full set of workflows. There should be no observable behavior changes. (With the exception of the log messages associated with the new output changes, and a warning message associated with the "known issue" indicated above.) Note that you'll also need the newly-saved output from the reduction workflow to test this PR.

The instrument associated with any output workspace can be examined directly. It should now have the same number of pixels as there are spectra in the workspace. Please see (and run through) the new CIS-test script at: tests/cis_tests/effective_instrument_script.py.

CIS testing

The new changes function automatically as part of the reduction process.
The instruments attached to the reduction-output workspaces should be examined, and verified to be as expected.
For comparison purposes, or in the case a work-around is required, the "application.yml" flag reduction.output.useEffectiveInstrument can be used to turn off the effective-instrument substitution.

Link to EWM item

EWM#6549

Verification

  • [ x] the author has read the EWM story and acceptance critera
  • the reviewer has read the EWM story and acceptance criteria
  • the reviewer certifies the acceptance criteria below reflect the criteria in EWM

Acceptance Criteria

  1. Where possible, all new changes are covered by unit tests.

  2. Reduction output workspaces will have a new effective instrument that has the same number of pixels as there are spectra in the workspace.  (And by assumption, these workspaces will have a number of spectra that correspond to the number of group-ids in the associated grouping.)

  3. The name of the effective instrument will be "SNAP_<grouping name>".

  4. Locations of each effective pixel will correspond to those of the unmasked PGP values for that pixel's group-id.  Note that tests/cis_tests/effective_instrument_script.py can be used to obtain and verify these values.  Mantid-workbench instrument view might also be a useful way to verify this quickly.

This list is for ease of reference, and does not replace reading the EWM story as part of the review. Verify this list matches the EWM story before reviewing.

  • acceptance criterion 1
  • acceptance criterion 2
  • acceptance criterion 3
  • acceptance criterion 4

@ekapadi
Copy link
Contributor Author

ekapadi commented Sep 27, 2024

I'm adding a cis-test script to help with verifying acceptance criterion #4.

@ekapadi ekapadi force-pushed the EWM6549_effective_instrument branch from 104f557 to a5f0817 Compare September 27, 2024 20:39
@ekapadi
Copy link
Contributor Author

ekapadi commented Oct 1, 2024

I ran into a snag with the acceptance criteria, so please wait to review this PR. There are some required Mantid changes in order to get the CIS-test to work. :(

@ekapadi ekapadi force-pushed the EWM6549_effective_instrument branch from 73f62a4 to db7ea42 Compare November 13, 2024 23:37
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.58%. Comparing base (b628382) to head (52b7143).
Report is 3 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #461      +/-   ##
==========================================
+ Coverage   96.55%   96.58%   +0.03%     
==========================================
  Files          66       67       +1     
  Lines        4960     5010      +50     
==========================================
+ Hits         4789     4839      +50     
  Misses        171      171              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekapadi ekapadi force-pushed the EWM6549_effective_instrument branch 7 times, most recently from a88b80f to 4267063 Compare November 15, 2024 18:41
@ekapadi
Copy link
Contributor Author

ekapadi commented Nov 15, 2024

Everything is now ready for review! :)

@walshmm
Copy link
Collaborator

walshmm commented Nov 19, 2024

Calibration and Normalization run without issue.
Reduction produces workspaces with the expected instrument name
but I'm not seeing any instrument in the instrument view on completion of reduction
image

@walshmm
Copy link
Collaborator

walshmm commented Nov 19, 2024

they reload with the new instrument but still none in the instrument viewer
image

@walshmm
Copy link
Collaborator

walshmm commented Nov 19, 2024

(the cis test script did pass with my data subbed in)

@ekapadi ekapadi force-pushed the EWM6549_effective_instrument branch 3 times, most recently from 413b15d to 35c7f34 Compare December 3, 2024 02:01
At the end of the reduction process, the instrument associated with each output workspace is modified.  A new _effective_ instrument is substituted for each workspace.  This instrument has the same number of pixels as there are group-ids, and the location of each pixel is set to the mean location of the _unmasked_ original pixels participating in that pixel group.  By implication, this substitution results in there being one pixel per spectrum in the output workspaces.

This commit includes the following changes:

  * A new `EffectiveInstrumentRecipe` implemented as a subrecipe called by `ReductionRecipe` for each grouping;

  * Modifications to `LocalDataService.writeReductionData` to use updated Mantid algorithms, now allowing limited I/O of programmatically-generated instruments;

  * Modification of `ReductionIngredients` to include the _unmasked_ `PixelGroup`s;

  * Modification of `SousChef.prepReductionIngredients` to prepare the _unmasked_ `PixelGroup`s;

  * Modification of existing unit tests, and implementation of new unit tests to verify the new subrecipe's execution.

Associated with this PR are three Mantid PRs, including changes to the `EditInstrumentGeometry`, `SaveNexusESS`, and `LoadNexusProcessed` algorithms.
@ekapadi ekapadi force-pushed the EWM6549_effective_instrument branch from 8bb61d7 to 28cc34f Compare December 3, 2024 22:12
@walshmm
Copy link
Collaborator

walshmm commented Dec 4, 2024

now with the flag off the assert correctly fails when checking to see if a new instrument geometry was applied to the output
image
However after activating the flag and rerunning the cis test it seems to fail, the instrument loaded on the workspace is None
image

@walshmm
Copy link
Collaborator

walshmm commented Dec 5, 2024

after rebuilding my conda env and reruning reduction it seemed to pass
image

# packages in environment at /SNS/users/wqp/.conda/envs/mamba/envs/SNAPRed:
#
# Name                    Version                   Build  Channel
mantid                    6.11.20241204.1559 py310h7c5db07_0    mantid/label/nightly
mantiddocs                6.11.20241204.1559      h2c57996_0    mantid/label/nightly
mantidqt                  6.11.20241204.1559 py310hb8bf3eb_0    mantid/label/nightly
mantidqtinterfaces        6.11.20241204.1559          pypi_0    pypi
mantidworkbench           6.11.20241204.1559 py310h38c35bf_0    mantid/label/nightly

@walshmm walshmm merged commit b42fc7b into next Dec 5, 2024
8 checks passed
@walshmm walshmm deleted the EWM6549_effective_instrument branch December 5, 2024 18:31
rboston628 pushed a commit that referenced this pull request Dec 6, 2024
* Reduction process: effective-instrument geometry.

At the end of the reduction process, the instrument associated with each output workspace is modified.  A new _effective_ instrument is substituted for each workspace.  This instrument has the same number of pixels as there are group-ids, and the location of each pixel is set to the mean location of the _unmasked_ original pixels participating in that pixel group.  By implication, this substitution results in there being one pixel per spectrum in the output workspaces.

This commit includes the following changes:

  * A new `EffectiveInstrumentRecipe` implemented as a subrecipe called by `ReductionRecipe` for each grouping;

  * Modifications to `LocalDataService.writeReductionData` to use updated Mantid algorithms, now allowing limited I/O of programmatically-generated instruments;

  * Modification of `ReductionIngredients` to include the _unmasked_ `PixelGroup`s;

  * Modification of `SousChef.prepReductionIngredients` to prepare the _unmasked_ `PixelGroup`s;

  * Modification of existing unit tests, and implementation of new unit tests to verify the new subrecipe's execution.

Associated with this PR are three Mantid PRs, including changes to the `EditInstrumentGeometry`, `SaveNexusESS`, and `LoadNexusProcessed` algorithms.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Kort Travis <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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