Skip to content

Conversation

@nvaytet
Copy link
Member

@nvaytet nvaytet commented Nov 4, 2025

In addition to #229 , we rename some more types (see commit messages for details).

I am not so sure about the WavelengthScaledQ -> NormalizedQ rename as some scaling happens for the Denominator but there isn't really a normalization going on for the Numerator...
Maybe we should just use a different name altogether?
I did feel that WavelengthScaledQ was not a good name anyway.

I was also unsure about the BinnedQ name. We go from WavelengthDetector to QDetector (after computing the Q coord but keeping detector dims). Then we bin in Q, so in principle the Detector part goes away, and we are left with just Q as the dim. In other techniques, the data is called Corrected<SOMETHING> at this point, so we still have the Corrected prefix, but we don't have it here.
So I just came up with BinnedQ.

@nvaytet nvaytet requested a review from jl-wynen November 4, 2025 09:34
@nvaytet
Copy link
Member Author

nvaytet commented Nov 4, 2025

I have a flaky test on my local machine:
tests/loki/iofq_test.py::test_pipeline_can_compute_IofQ[False-UncertaintyBroadcastMode.drop].

Full error is

=================================================== FAILURES ====================================================
______________________ test_pipeline_can_compute_IofQ[False-UncertaintyBroadcastMode.drop] ______________________

uncertainties = <UncertaintyBroadcastMode.drop: 1>, qxy = False

    @pytest.mark.parametrize(
        'uncertainties',
        [UncertaintyBroadcastMode.drop, UncertaintyBroadcastMode.upper_bound],
    )
    @pytest.mark.parametrize('qxy', [False, True])
    def test_pipeline_can_compute_IofQ(uncertainties, qxy: bool):
        pipeline = make_workflow(no_masks=False)
        pipeline[UncertaintyBroadcastMode] = uncertainties
        pipeline = sans.with_pixel_mask_filenames(
            pipeline, loki.data.loki_tutorial_mask_filenames()
        )
        pipeline[BeamCenter] = sans.beam_center_from_center_of_mass(pipeline)
        if qxy:
            result = pipeline.compute(BackgroundSubtractedIofQxy)
            assert result.dims == ('Qy', 'Qx')
            assert sc.identical(result.coords['Qx'], pipeline.compute(QxBins))
            assert sc.identical(result.coords['Qy'], pipeline.compute(QyBins))
            assert result.sizes['Qx'] == 90
            assert result.sizes['Qy'] == 77
        else:
            result = pipeline.compute(BackgroundSubtractedIofQ)
            assert result.dims == ('Q',)
            assert sc.identical(result.coords['Q'], pipeline.compute(QBins))
            assert result.sizes['Q'] == 100
        if uncertainties == UncertaintyBroadcastMode.drop:
            test_dir = os.path.dirname(os.path.abspath(__file__))
            name = Path(f'reference_IofQ{"xy" if qxy else ""}_{uncertainties}.hdf5')
            reference = sc.io.load_hdf5(test_dir / name)
>           assert_identical(result, reference)

tests/loki/iofq_test.py:87: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../path/scipp/testing/assertions.py:99: in assert_identical
    return _assert_similar(_assert_identical_impl, a, b)
../../path/scipp/testing/assertions.py:104: in _assert_similar
    impl(*args, **kwargs)
../../path/scipp/testing/assertions.py:127: in _assert_identical_impl
    _assert_identical_data(a, b)
../../path/scipp/testing/assertions.py:204: in _assert_identical_data
    _assert_identical_variable_data(a.data, b.data)
../../path/scipp/testing/assertions.py:217: in _assert_identical_variable_data
    _assert_identical_dense_variable_data(a, b)
../../path/scipp/testing/assertions.py:222: in _assert_identical_dense_variable_data
    np.testing.assert_array_equal(
../../../miniforge3/lib/python3.12/site-packages/numpy/_utils/__init__.py:85: in wrapper
    return fun(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (<built-in function eq>, array([4.2163105 , 4.616472  , 4.709668  , 4.601788  , 4.390278  ,
       4.0894866 , 3.74352...8204, 0.2738136 , 0.2516059 ,
       0.26882824, 0.2726485 , 0.28334573, 0.25686255, 0.25695458],
      dtype=float32))
kwds = {'err_msg': 'when comparing values', 'header': 'Arrays are not equal', 'strict': False, 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError: 
E           Arrays are not equal
E           when comparing values
E           Mismatched elements: 11 / 100 (11%)
E           Max absolute difference among violations: 4.7683716e-07
E           Max relative difference among violations: 1.1717833e-07
E            ACTUAL: array([4.216311, 4.616472, 4.709668, 4.601788, 4.390278, 4.089487,
E                  3.743527, 3.373496, 3.044424, 2.733558, 2.482377, 2.240014,
E                  2.070589, 1.893589, 1.749089, 1.633279, 1.532102, 1.421638,...
E            DESIRED: array([4.216311, 4.616472, 4.709669, 4.601788, 4.390278, 4.089487,
E                  3.743527, 3.373496, 3.044424, 2.733558, 2.482377, 2.240015,
E                  2.070589, 1.893589, 1.749089, 1.633279, 1.532102, 1.421638,...
E           in values

../../../miniforge3/lib/python3.12/contextlib.py:81: AssertionError

@jl-wynen
Copy link
Member

jl-wynen commented Nov 4, 2025

I have a flaky test on my local machine:
tests/loki/iofq_test.py::test_pipeline_can_compute_IofQ[False-UncertaintyBroadcastMode.drop].

Is there randomness in the workflow? Or is this due to multi threading?

@nvaytet
Copy link
Member Author

nvaytet commented Nov 4, 2025

My guess is multi-threading... The tests passes if I replace identical with allclose.

@jl-wynen
Copy link
Member

jl-wynen commented Nov 4, 2025

I am not so sure about the WavelengthScaledQ -> NormalizedQ rename as some scaling happens for the Denominator but there isn't really a normalization going on for the Numerator...
Maybe we should just use a different name altogether?
I did feel that WavelengthScaledQ was not a good name anyway.

I agree. Why do we have the same type for numerator and denominator? It seems odd to use a generic here because the two represent different things conceptually.

I was also unsure about the BinnedQ name. We go from WavelengthDetector to QDetector (after computing the Q coord but keeping detector dims). Then we bin in Q, so in principle the Detector part goes away, and we are left with just Q as the dim. In other techniques, the data is called Corrected at this point, so we still have the Corrected prefix, but we don't have it here.
So I just came up with BinnedQ.

You could carry the Corrected prefix through from CorrectedDetector. But I don't love the idea. Is there a reason this workflow first corrects and then computes wavelength and Q? Other workflows do it the other way around.

@nvaytet
Copy link
Member Author

nvaytet commented Nov 4, 2025

Is there a reason this workflow first corrects and then computes wavelength and Q? Other workflows do it the other way around.

This is the question I spent a lot of time on during the retreat... but for now I decided I don't want to touch it because I am bound to break something somewhere...

class MaskedSolidAngle(sciline.Scope[ScatteringRunType, sc.DataArray], sc.DataArray):
"""Same as :py:class:`SolidAngle`, but with pixel masks applied"""
# class MaskedSolidAngle(sciline.Scope[ScatteringRunType, sc.DataArray], sc.DataArray):
# """Same as :py:class:`SolidAngle`, but with pixel masks applied"""
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

@nvaytet nvaytet merged commit 7783fc1 into standard-domain-types Nov 4, 2025
4 checks passed
@nvaytet nvaytet deleted the standard-domain-types3 branch November 4, 2025 13:10
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.

3 participants