Fixes bug with filter.matchedfilter.optimized_match

[bmck039]

This PR fixes a bug in filter.matchedfilter.optimized_match that causes the return of incorrect match values. The code originally did not respect the minimization bounds, causing local minima to be incorrectly returned in some rare instances.

Standard information about the request

This is a: bug fix

This change changes: scientific output

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

This change will: Fix a bug described in #5144

Motivation

Incorrect match values were encountered when calculating the match over different wave parameters. In these cases, the match discontinuously jumped as described in #5144.

Contents

• matchedfilter.py: optimized_match(): Changes scipy minimize_scalar method to one that respects minimization bounds
• test_matchedfilter.py: test_optimized_match_valid(): Adds to existing automated tests by using a known-tricky waveform to ensure optimized_match() calculates the correct value

Links to any issues or associated PRs

Fixes #5144

Testing performed

Extremely simple change, no testing required.

Additional notes

• The author of this pull request confirms they will adhere to the code of conduct

[jacopok]

Thanks for the PR!

You can see here why it's failing: you also need to change bracket=(-delta_t, delta_t) to bounds=(-delta_t, delta_t) (don't know why they made the syntax different)

[bmck039]

Thanks for the reminder, it should be updated now to reflect that.

[bmck039]

Seems it's failing the accuracy checks now by 1 decimal place each time. The scipy documentation states that the "Bounded" method uses the same underlying algorithm as "Brent" so I'm not sure why the resulting values would be different.

[jacopok]

Hm, interesting, I don't really know why that is the case.

I'll note that the quantity failing the accuracy check is not o, the match, but i, quantifying the timeshift (to achieve the match, the waveforms need to be shifted by i * Deltat).

It is possible to pass the xatol argument to the solver; maybe that will fix it?

Anyhow, the 4-decimal-figures accuracy requirement is arbitrary, and it's possible that the solver was previously achieving it "by chance".

[jacopok]

I don't exactly understand why the test is failing and don't really have the bandwidth to look into it at the moment.

I think the way to go here could be to relax the requirement in the test suite to 3 decimal places and perhaps allow the user to pass **kwargs to the optimized_match function, which then could be passed to the optimizer call, as @ahnitz originally suggested.