Move empty can subtraction to end #170
Conversation
src/ess/powder/correction.py
Outdated
| def subtract_background( | ||
| data: FocussedDataDspacing[SampleRun], | ||
| data: IofDspacing, | ||
| background: FocussedDataDspacing[BackgroundRun], |
There was a problem hiding this comment.
Shoudn't this be IofDspacing[BackgroundRun]? In other words, don' we need to parametrize IofDspacing?
There was a problem hiding this comment.
Done. But this required changing IofDspacing to a generic type. So this is a breaking change to the workflow which needs to request IofDspacing[SampleRun] now. I had coded SampleRun in some places to avoid errors from using the same type twice as an argument. E.g.,
def subtract_background(
data: IofDspacing[RunType],
background: IofDspacing[BackgroundRun],
) -> EmptyCanSubtractedIofDspacing[RunType]:
...Would result in an instantiation that uses IofDspacing[BackgroundRun] for both arguments and Sciline doesn't like that.
There was a problem hiding this comment.
Given that tests passed with this "bug" (before your latest changes): Is there a test missing that actually checked the result of the background subtraction?
There was a problem hiding this comment.
We don't have any regression tests for the final result. But they would have been broken in this case anyway.
I don't know how we can test that the background subtraction is applied correctly. Unfortunately, the data are dimensionless at this point. Do you have an idea?
There was a problem hiding this comment.
Check that the workflow gives (A - B)/C, locally?
1864902 to
1ddd7c5
Compare
1ddd7c5 to
9874cc5
Compare
| def test_pipeline_normalizes_and_subtracts_empty_can_as_expected( | ||
| workflow: sciline.Pipeline, | ||
| ) -> None: | ||
| workflow[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop | ||
| workflow = powder.with_pixel_mask_filenames(workflow, []) | ||
| results = workflow.compute( | ||
| [ | ||
| EmptyCanSubtractedIofDspacing[SampleRun], | ||
| FocussedDataDspacing[SampleRun], | ||
| FocussedDataDspacing[VanadiumRun], | ||
| FocussedDataDspacing[BackgroundRun], | ||
| ] | ||
| ) | ||
| result = results[EmptyCanSubtractedIofDspacing[SampleRun]] | ||
|
|
||
| sample = results[FocussedDataDspacing[SampleRun]] | ||
| empty_can = results[FocussedDataDspacing[BackgroundRun]] | ||
| vanadium = results[FocussedDataDspacing[VanadiumRun]] | ||
|
|
||
| expected = powder.correction.normalize_by_vanadium_dspacing( | ||
| sample.bins.concatenate(-empty_can), vanadium, UncertaintyBroadcastMode.drop | ||
| ) | ||
| sc.testing.assert_allclose(result, expected) |
There was a problem hiding this comment.
Ok, but I thought we wanted to avoid always running a full workflow in every "unit" test, due to growing run times... 😬
There was a problem hiding this comment.
Good point. This was easy enough to change.
This should make working with live data more efficient.