Ewm6834 fix default record #452

walshmm · 2024-09-04T13:22:15Z

Description of work

This pr allows:

snapred to utilize the default calib record written out as part of state init
reduction to refer to the correct calibrant sample when creating peaks to purge from normalization

This needs a follow up to address when a state is totally uninitialized.

Explanation of work

I brought over some changes from staging for the calibrant sample stuff. These were actually somewhat essential when interacting with the default record, as it doesnt have a calibrant sample set (nor is it relevant to reduction).

I updated GroceryService and Indexer to account for the default record, adding a case to attempt to read on if no other record is available. Doing this, I also changed the way we pull diffcaltable names, and refer to the calibration record filtering on ws type--this simplified referring to the special name default tables recieve and allow me to avoid special logic in this case.

To test

Initialize a new state.
run reduction
it should prompt you to continue anyway
see that it completes

For reduction/calib
Runnumber: 59039 

for calib:
sample: LA11b6
Grouping: Column
Peak Intensity Thresh: 0.01
Chi: 200
dmin: 1

for norm:
rnunumber: 58810
bgrunnumber: 58813

this should apply the default calibration during reduction.

Link to EWM item

EWM#6834

codecov · 2024-09-05T13:51:28Z

Codecov Report

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

Project coverage is 96.11%. Comparing base (f9f15f9) to head (15509a6).
Report is 73 commits behind head on next.

Files with missing lines	Patch %	Lines
src/snapred/backend/service/SousChef.py	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #452      +/-   ##
==========================================
+ Coverage   96.08%   96.11%   +0.03%     
==========================================
  Files          61       61              
  Lines        4445     4483      +38     
==========================================
+ Hits         4271     4309      +38     
  Misses        174      174

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

ekapadi · 2024-09-12T15:56:08Z

src/snapred/backend/dao/request/CreateNormalizationRecordRequest.py


-
-class CreateNormalizationRecordRequest(BaseModel, extra="forbid"):
+class CreateNormalizationRecordRequest(NormalizationRecord, extra="forbid"):


I do see the purpose of this, but the necessity for this extra layer struck me as a bit weird. That is, if this is basically just a NormalizationRecord, then why do we need the additional request class. (For example, instead we could have Indexer.updateRecordVersions, instead of Indexer.createRecord(<from this record??>) which might be more clear, in terms of what is actually happening.)

I did think this data object was kinda odd, I didnt look too closely at it. Looking further into how its used there's a bit of a smell.
I think this had been just passing a record around up until the indexer stuff?
I'll see if I can rectify this.

ekapadi · 2024-09-12T15:57:49Z

src/snapred/backend/data/GroceryService.py

@@ -222,11 +224,19 @@ def _createDiffcalDiagnosticWorkspaceFilename(self, item: GroceryListItem) -> st
            / (self._createDiffcalOutputWorkspaceName(item) + ext)
        )

+    def _createDiffcalTableFilepathFromWsName(


I don't understand this. I could do
runNumber, useLiteMode, version = wsName.tokens('runNumber', 'useLiteMode', 'version')
to get the required field values. What is the purpose of the redundant arguments?

Also, and possibly more importantly, what happens when these argument values are in contradiction with each other?

ekapadi · 2024-09-12T16:05:47Z

src/snapred/backend/data/LocalDataService.py

@@ -842,7 +842,7 @@ def _writeDefaultDiffCalTable(self, runNumber: str, useLiteMode: bool):
        indexer = self.calibrationIndexer(runNumber, useLiteMode)
        version = indexer.defaultVersion()
        grocer = GroceryService()
-        filename = Path(grocer._createDiffcalTableWorkspaceName("default", useLiteMode, version) + ".h5")
+        filename = Path(grocer.createDiffcalTableWorkspaceName("default", useLiteMode, version) + ".h5")


I don't get this: why does "default" subsume the <run number> position (especially as "default" as a string is not a legitimate <run number>), but not just use the VERSION_DEFAULT which is provided for that reason particularly? (And then use the real <run number> which might be useful information. What am I missing here?)

I realized later that possibly there's no meaningful <run number> where this is actually required. I would like to see these "default" names centralized somewhere (we have the same requirement for < version > ), but that doesn't have much to do with this PR.

ekapadi · 2024-09-12T16:08:53Z

tests/integration/test_versions_in_order.py

@@ -199,7 +199,7 @@ def fetchCalibrationWorkspaces(self, item: Any) -> Dict[str, Any]:
        }

    def fetchDefaultDiffCalTable(self, runNumber: str, useLiteMode: bool, version: int | Any) -> str:
-        tableWorkspaceName = self._createDiffcalTableWorkspaceName("default", useLiteMode, version)
+        tableWorkspaceName = self.createDiffcalTableWorkspaceName("default", useLiteMode, version)


Same issue as mentioned previously: this name should possibly not be generated this way.

ekapadi · 2024-09-12T16:10:55Z

src/snapred/backend/dao/normalization/NormalizationRecord.py

@@ -33,6 +33,7 @@ class NormalizationRecord(Record, extra="ignore"):
    workspaceNames: List[WorkspaceName] = []
    calibrationVersionUsed: int = VERSION_DEFAULT
    crystalDBounds: Limit[float]
+    normalizationCalibrantSamplePath: str


I'm tempted to suggest, that because this is a NormalizationRecord, the "normalization" part of normalizationCalibrantSamplePath is redundant. And that to clarify the usage, the "normalization" only be added at the ReductionRecord.normalizationCalibrantSamplePath. (However, I can go either way on this.)

ekapadi · 2024-09-12T16:13:31Z

tests/util/WhateversInTheFridge.py

@@ -100,6 +100,7 @@ def readNormalizationRecord(self, runId: str, useLiteMode: bool, version: Option
            useLiteMode=useLiteMode,
            version=version,
            calculationParameters=self.calculationParameters_with_stateId("0xdeadbeef"),
+            normalizatinoCalibrantSamplePath="path/to/calibrant",


Misspelling of normalizationCalibrationSamplePath (and also check it at the source in the NormalizationRecord declaration).

fix default calibration use during reduction fix some tests fix the rest of the tests remove comment, no longer relevant

…ecords

…Normalization'.

ekapadi · 2024-10-03T20:47:09Z

I tested reduction, following only state initialization (i.e. no calibration). Everything works as it should. The resulting ReductionRecord indicates both that the default calibration was used (with the new name for the default calibration table), and that no normalization was used.

Then I tested the calibration and normalization panels: in normalization save panel, there was a bug (as the new normalizationCalibrantSamplePath wasn't being set in NormalizationWorkflow._saveNormalization. (AND, a key question, why aren't most of the internals of this method part of the NormalizationService?!)

Performed a minor rebase, fixed the bug, and retested. Everything appears to be working as it should be.
I "approve" this PR and it should be ready to merge, once the "read the docs" thing resolves.

However Darsh will actually need to approve it, since I modified the code.

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

darshdinger

Looks good. Reviewed and tested.

walshmm force-pushed the ewm6834_fix_default_record branch 2 times, most recently from 3c10247 to 6973dbc Compare September 5, 2024 13:49

walshmm force-pushed the ewm6834_fix_default_record branch from f97d9d1 to 0603a92 Compare September 9, 2024 19:54

walshmm requested a review from ekapadi September 10, 2024 20:40

ekapadi reviewed Sep 12, 2024

View reviewed changes

walshmm added 5 commits October 3, 2024 16:11

implement fix for default calibration reading

796093e

fix default calibration use during reduction fix some tests fix the rest of the tests remove comment, no longer relevant

up test coverage

5abc3a6

reorg to verify generated names against stored names in calibration r…

cfbf802

…ecords

forgot an f

1534d9a

fix spelling mistake

cafe2e9

ekapadi force-pushed the ewm6834_fix_default_record branch from 57046cd to cafe2e9 Compare October 3, 2024 20:12

Set 'normalizationCalibrantSamplePath' at 'NormalizationWorkflow.save…

db0bb2e

…Normalization'.

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

15509a6

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

darshdinger approved these changes Oct 3, 2024

View reviewed changes

ekapadi merged commit f5aec1f into next Oct 3, 2024
7 checks passed

ekapadi deleted the ewm6834_fix_default_record branch October 3, 2024 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ewm6834 fix default record #452

Ewm6834 fix default record #452

Uh oh!

walshmm commented Sep 4, 2024

Uh oh!

codecov bot commented Sep 5, 2024 •

edited

Loading

Uh oh!

ekapadi Sep 12, 2024

Uh oh!

walshmm Sep 12, 2024 •

edited

Loading

Uh oh!

ekapadi Sep 12, 2024

Uh oh!

ekapadi Sep 12, 2024

Uh oh!

ekapadi Sep 12, 2024 •

edited

Loading

Uh oh!

ekapadi Sep 13, 2024

Uh oh!

ekapadi Sep 12, 2024

Uh oh!

ekapadi Sep 12, 2024 •

edited

Loading

Uh oh!

ekapadi Sep 12, 2024 •

edited

Loading

Uh oh!

ekapadi commented Oct 3, 2024 •

edited

Loading

Uh oh!

darshdinger left a comment

Uh oh!

Uh oh!

Uh oh!



		class CreateNormalizationRecordRequest(BaseModel, extra="forbid"):
		class CreateNormalizationRecordRequest(NormalizationRecord, extra="forbid"):

Ewm6834 fix default record #452

Ewm6834 fix default record #452

Uh oh!

Conversation

walshmm commented Sep 4, 2024

Description of work

Explanation of work

To test

Link to EWM item

Uh oh!

codecov bot commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ekapadi Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

walshmm Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekapadi Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekapadi commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darshdinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 5, 2024 •

edited

Loading

walshmm Sep 12, 2024 •

edited

Loading

ekapadi Sep 12, 2024 •

edited

Loading

ekapadi Sep 12, 2024 •

edited

Loading

ekapadi Sep 12, 2024 •

edited

Loading

ekapadi commented Oct 3, 2024 •

edited

Loading