Dispersion error propagation was using wrong values

[mattiastefanelli]

In Dispersion._compute_elements, the call to propagate_error_dispersion was incorrectly passing model_disp (the model dispersion) as its first argument. According to the propagation formula (see below), this argument must be the model beta function. The fix replaces model_disp with model_beta extracted from the segment model.

The current implementation covers only the first term of the full dispersion uncertainty.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
omc3/segment_by_segment/propagables
dispersion.py					117-125
Project Total

This report was generated by python-coverage-comment-action

[jgray-19]

Nice PR. I assume you have some checks that proves this is correct? If so, could be nice to include.

@fsoubelet. If adding tests is not a priority would you be happy to merge despite the coverage failure?? I'm hesitant to do this especially if the fix isn't very urgent.

Also I haven't looked in the code, but please make sure all the equations used are documented.

[JoschD]

• Remove comment in fixed line
• Fix function arguments in the other places

[mattiastefanelli]

def propagate_error_dispersion(beta: ArrayLike, dphi: ArrayLike, init: SegmentBoundaryConditions) -> ArrayLike:
    """Propagates the dispersion error with dphi phase-advance.

    Args:
        beta (ArrayLike): Beta function at the observation point(s).
        dphi (ArrayLike): Phase advances from the initial position to the observation point(s), in units of 2pi radians.
        init (PropagableBoundaryConditions): Initial conditions for alpha and beta and the dispersion uncertainty.
    """
    _, errdispersion0 = init.dispersion
    beta0, _ = init.beta
    alpha0, _ = init.alpha

    return np.abs(
        errdispersion0 * np.sqrt(beta / beta0) *
        (np.cos(2 * np.pi * dphi) + alpha0 * np.sin(2 * np.pi * dphi))
    )

the main function for the dispersion error was already expecting beta (beta (ArrayLike): Beta function at the observation point(s)), I removed the comment in fixed code line.

[fsoubelet]

I'd be interested in seeing a before and after if possible, this should be a big change of the error bars considering the values difference in beta vs disp.

This does not feel urgent (especially with the schedule this year) so I would like to see a test added with new truth values.

Also, thanks @mattiastefanelli for re-deriving the expressions, we can already see that it was useful beyong safekeeping :)

[mattiastefanelli]

I am also preparing some notes for the error propagation equations. So that we have a document to use as reference for future works. @jgray-19

[jgray-19]

Yes I think a test would be nice. You could create a finite difference test, which would also test the accuracy of your derivations?

[mattiastefanelli]

Thanks @fsoubelet! The errors indeed increase considering the betas instead of the dispersions in the equation. But the main thing for which I noticed the bug is the fact that we were getting NaNs when the dispersion become negative which was strange. In the plot the red 'x' are pointing to points with NaN as errors for a test segment. The screenshot gives an example of the increase in the error estimation.