Ewm7732 normalize by direct beam monitor #550

walshmm · 2025-02-21T19:11:31Z

Description of work

This adds a toggle to tell snapred to normalize by events from the monitor rather than proton charge.

Explanation of work

I refactored GroceryService to remove the two complex case statements, and instead model a more accurate fallback structure for loading native/lite/live data.

I then added the loading of monitor workspaces to the end, save the total events of the processed monitor workspace as a log, and use said log in normalization calculations down the line.

To test

Run each workflow, ensure that they still work with the toggle off.
Record results as far as normalization by current is concerned( like the log 'NormalizationFactor')
Run them again, ensure that they work with the toggle on.
Observe the monitor workspace get loaded and then quickly deleted during load.
Compare the normalization factor this time around, confirm that they are different and that the toggle was effective.

Ensure the results of each workflow are as expected, no out of the ordinary graphs for the normal testing data.

Link to EWM item

EWM#7732

Verification

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

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.

There is a toggle in the application.yml for enabling this flow, and a config for which monitor to use (defaults false, and 0)
Workspaces have additional logs indicating that this was their method of normalization
The workspaces are in fact normalized this way (i.e. different results in the case of reduction, normalization, ect)
The story mentions needing different 'width'/'L1'/'particleBouncs' for the RemovePromptPulse on the monitor workspace but did not provide a location for said width, however using the same ones provided normally seemed to yield decent results.

codecov · 2025-02-24T17:33:47Z

Codecov Report

Attention: Patch coverage is 99.23077% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.09%. Comparing base (eca6ddc) to head (cc056d9).
Report is 1 commits behind head on next.

Files with missing lines	Patch %	Lines
src/snapred/backend/data/GroceryService.py	99.21%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #550      +/-   ##
==========================================
- Coverage   96.10%   96.09%   -0.01%     
==========================================
  Files          71       71              
  Lines        5466     5458       -8     
==========================================
- Hits         5253     5245       -8     
  Misses        213      213

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

rebase and fix unit tests cleanup reabse [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci implement normalize by monitor fix some tests after a rebase missed a test completed reduction with normalizebymonitor fix existing tests

…o make more sense

rboston628

I still need to look at the GroceryService changes

tests/unit/backend/data/test_DataFactoryService.py

src/snapred/backend/recipe/algorithm/RawVanadiumCorrectionAlgorithm.py

src/snapred/backend/recipe/ReductionGroupProcessingRecipe.py

src/snapred/backend/data/GroceryService.py

src/snapred/backend/recipe/algorithm/NormalizeByCurrentButTheCorrectWay.py

ekapadi · 2025-02-28T14:52:27Z

One question about how you recommend to compare the normalization factors? A step-by-step might be useful. -- Thanks!

ekapadi · 2025-02-28T18:40:43Z

Monitor WNG workspace-name format is coming up as monitor_false... which is inconsistent with how the "lite-mode" token is formafted for run -- I think the solution is to initialize your WNG.monitor method in exactly the same way as the run method. (And possibly, not to fix anything beyond that for this PR... :) )

ekapadi · 2025-02-28T18:45:50Z

Without using monitor: NormalizationFactor = 2777.79

With monitor: NormalizationFactor = 1.465e-7 ??? This seems way too large of a change to me. Do you have a simple way to explain why this is what I should be expecting?
NormalizationFactor * ..normalizeByMonitorFactor ~= 1.0

walshmm · 2025-02-28T18:52:30Z

Without using monitor: NormalizationFactor = 2777.79

With monitor: NormalizationFactor = 1.465e-7 ??? This seems way to0 large of a change to me. Do you have a simple way to explain why this is what I should be expecting? NormalizationFactor * ..normalizeByMonitorFactor ~= 1.0

I could be recording the wrong value? I may have mistook NormalizeByCurrent as saving the actual scaling factor used as opposed to proton charge(?) I'll take a look.

walshmm · 2025-02-28T18:54:38Z

Though this is a little weird as far as the background workspace is concerned, as we are not doing a typical normalization? its some ratio of background to vanadium run monitor counts. This would be difficult to record in a number close to 2777 I think?

walshmm · 2025-02-28T18:55:15Z

The purpose of saving it to NormalizationFactor is so that later down the line it is not normalized again...

ekapadi

I added a few comments as "one offs" so I could do the review a little bit at a time. So far the only issues are with the WNG-formatted name of the monitor workspace, and with the extreme difference between the NormalizationFactor in the two modes. All of the workflows work as expected (including the live-data workflow). I'm looking at the grocery-service changes next...

ekapadi · 2025-02-28T18:56:25Z

Now I'm also wondering if this monitor-based normalization mode applies to the "art norm" workflow as well?

walshmm · 2025-02-28T18:57:58Z

Now I'm also wondering if this monitor-based normalization mode applies to the "art norm" workflow as well?

I dont think so? The art norm is cloned off the reduction input workspace after normalizeByCurrent/Monitor, so there is no background and its already normalized I think?

ekapadi

I really like the new flow-control changes in GroceryService -- it tremendously simplifies things! I left a few comments about remaining questions that I have, but once those are dealt with I am fine with all of these changes. (Provided we set "liveData.enabled" to False for the moment -- I don't think the mutex error is caused by anything in this PR.)

ekapadi · 2025-02-28T20:08:09Z

src/snapred/backend/dao/ingredients/GroceryListItem.py

@@ -56,6 +56,7 @@ class GroceryListItem(BaseModel):
        "",
        "LoadCalibrationWorkspaces",
        "LoadEventNexus",
+        "LoadNexusMonitors",


Suggest: in alphabetical order -- that's what I had been doing here. :)

ekapadi · 2025-02-28T20:09:33Z

src/snapred/backend/data/GroceryService.py

-        convertToLiteMode = False
+        # 1. check cache
+        # 2. check if file exists
+        # e. cannot find data.


'e.' or '3.'?

e for error, fallback case.

ekapadi · 2025-02-28T20:10:38Z

src/snapred/backend/data/GroceryService.py


-        self._updateNeutronCacheFromADS(runNumber, useLiteMode)
+        data["workspace"] = workspaceName
+        # NOTE: When would it ever be false?  If the workspace is not found, it should raise an error.


We are not necessarily consistent about this. Sometimes the return value is used, and sometimes it's not. This is not a good thing!

ekapadi · 2025-02-28T20:11:33Z

src/snapred/backend/data/GroceryService.py

-                    data = self.grocer.executeRecipe(str(nativeModeFilePath), workspaceName, loader)
-                    success = True
+    def _fetchNeutronDataNative(self, item: GroceryListItem) -> Dict[str, Any]:
+        item.useLiteMode = False


Changing the value on the incoming item isn't a good idea. Definitely you need to make a copy here!

ekapadi · 2025-02-28T20:12:23Z

src/snapred/backend/data/GroceryService.py

+        return self._fetchNeutronDataSingleUse(item, tryLiveData)
+
+    def _fetchNeutronDataLite(self, item: GroceryListItem, export=False) -> Dict[str, Any]:
+        item.useLiteMode = True


Same comment as previous. It makes it really difficult to trace what is happening. So definitely make a copy of the item before modifying it.

ekapadi · 2025-02-28T20:18:51Z

src/snapred/backend/data/GroceryService.py

+        workspaceName = self._createMonitorWorkspaceName(runNumber, useLiteMode)
+
+        # TODO: Cache if necessary
+        if self.workspaceDoesExist(workspaceName):


Do we do this type of caching for any other type of workspace? If not, I think we need to be consistent about how we cache. That is, you probably need a _loadedMonitors dict.

ekapadi · 2025-02-28T20:19:59Z

src/snapred/backend/data/GroceryService.py

@@ -1490,7 +1432,7 @@ def getResidentWorkspaces(self, excludeCache: bool):
            workspaces = workspaces.difference(self.getCachedWorkspaces())
        return list(workspaces)

-    def _processNeutronDataCopy(self, runNumber, workspaceName):
+    def _filterEvents(self, runNumber, workspaceName):


Are we moving the removePromptPulse here as well, or is that part of another story?

prompt pulse has already been moved here, that change is in next.

ekapadi · 2025-02-28T20:23:29Z

src/snapred/backend/recipe/algorithm/MantidSnapper.py

@@ -34,6 +34,9 @@ def unique_name(self, n=5, prefix="", suffix=""):
    def unique_hidden_name(self):
        return mtd.unique_hidden_name()

+    def getSNAPRedLog(self, wsName, logname):
+        return self[wsName].getRun().getLogData(f"{Config['metadata.tagPrefix']}{logname}").value


I really, really want this to throw a KeyError if the log isn't there. If I'm remembering correcty, that's not what mantid.api.Run does. Can we please wrap this in a try?

ekapadi · 2025-02-28T20:25:11Z

src/snapred/backend/recipe/algorithm/NormalizeByCurrentButTheCorrectWay.py

@@ -1,13 +1,17 @@
 from mantid.api import AlgorithmFactory, MatrixWorkspaceProperty, PropertyMode, PythonAlgorithm
 from mantid.kernel import Direction
-from mantid.simpleapi import CloneWorkspace, NormaliseByCurrent, mtd
+from mantid.simpleapi import CloneWorkspace, NormaliseByCurrent, Scale


Why are we using MantidSnapper inconsistently here?

ekapadi · 2025-02-28T20:27:55Z

src/snapred/resources/application.yml

@@ -157,6 +157,9 @@ reduction:

 mantid:
  workspace:
+    monitor:
+      normalizeByBeam: false
+      normID: 0


**I suggest expanding these names slightly (e.g.) normalizeByBeamMonitor and normalizationMonitorID (**or just monitorId). Also we should check whether it's "Id" or "ID", to be consistent.

walshmm force-pushed the ewm7732_normalize_by_direct_beam_monitor branch 2 times, most recently from bdf41fe to fb99283 Compare February 24, 2025 17:31

walshmm force-pushed the ewm7732_normalize_by_direct_beam_monitor branch 3 times, most recently from c753f3e to 16526a1 Compare February 25, 2025 19:50

walshmm force-pushed the ewm7732_normalize_by_direct_beam_monitor branch from 82e17dd to 22fa04c Compare February 26, 2025 17:00

walshmm added 6 commits February 26, 2025 14:08

turn back on cropping for integration tests

f120544

up coverage

4cec9bc

incorrectly fixed a ruff error

990ec1b

fix submodule version?

60bc85d

missed another line

4249637

updated how nomalize by monitor is applied in rawvanadiumcorrection t…

72fb959

…o make more sense

walshmm marked this pull request as ready for review February 27, 2025 16:15

rboston628 reviewed Feb 27, 2025

View reviewed changes

response to comments

cc056d9

ekapadi reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ewm7732 normalize by direct beam monitor #550

Ewm7732 normalize by direct beam monitor #550

walshmm commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 24, 2025 •

edited

Loading

rboston628 left a comment

ekapadi commented Feb 28, 2025

ekapadi commented Feb 28, 2025

ekapadi commented Feb 28, 2025 •

edited

Loading

walshmm commented Feb 28, 2025

walshmm commented Feb 28, 2025

walshmm commented Feb 28, 2025

ekapadi left a comment

ekapadi commented Feb 28, 2025

walshmm commented Feb 28, 2025

ekapadi left a comment

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

walshmm Feb 28, 2025

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

walshmm Feb 28, 2025

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

ekapadi Feb 28, 2025

Ewm7732 normalize by direct beam monitor #550

Are you sure you want to change the base?

Ewm7732 normalize by direct beam monitor #550

Conversation

walshmm commented Feb 21, 2025 • edited Loading

Description of work

Explanation of work

To test

Link to EWM item

Verification

Acceptance Criteria

codecov bot commented Feb 24, 2025 • edited Loading

Codecov Report

rboston628 left a comment

Choose a reason for hiding this comment

ekapadi commented Feb 28, 2025

ekapadi commented Feb 28, 2025

ekapadi commented Feb 28, 2025 • edited Loading

walshmm commented Feb 28, 2025

walshmm commented Feb 28, 2025

walshmm commented Feb 28, 2025

ekapadi left a comment

Choose a reason for hiding this comment

ekapadi commented Feb 28, 2025

walshmm commented Feb 28, 2025

ekapadi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walshmm commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 24, 2025 •

edited

Loading

ekapadi commented Feb 28, 2025 •

edited

Loading