Improve passivity enforcement near high-Q poles in FastDispersionFitter

[caseyflex]

The passivity enforcement including imag_ep_extrema and omega_range was not sufficient when the fit PoleResidue model contained high-Q poles. This PR adds extra samples around each pole in order to improve the passivity enforcement. Specifically, the samples are computed using an explicit formula for extrema around a single pole.

The fit for the example data in the issue remains poor -- this is to be expected, since the data exhibits dispersion but has no loss.

[alec-flexcompute]

Looks good if it passes this test! Not sure how the fixes you made exactly work but it looks reasonable :)

[caseyflex]

I’m still checking over some things. This may discard too many models if the criterion is just based on high Q.

…

On Thu, Aug 8, 2024 at 3:34 PM flex-Alec ***@***.***> wrote: ***@***.**** approved this pull request. Looks good if it passes this test! Not sure how the fixes you made exactly work but it looks reasonable :) — Reply to this email directly, view it on GitHub <#1894 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3KLECIFINIOJWTX7DL7YCTZQN6XNAVCNFSM6AAAAABMGLHGF6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMRYGA3TONRXGU> . You are receiving this because you were assigned.Message ID: ***@***.***>

[caseyflex]

OK, I'm happy with this now. I fixed the formula and am no longer rejecting models on the basis of being high-Q. This PR just adds one or two extra samples around each pole, except for marginally-stable poles, the handling of which is not changed.

It might be worth checking some of the material_library fits to see if anything changed (indicating that previously the fit wasn't actually passive)

[alec-flexcompute]

ah, this is a good point. Nice work @caseyflex!

[caseyflex]

ah, this is a good point. Nice work @caseyflex!

thanks for finding the issue!

[weiliangjin2021]

Nice work! When I tried Alec's medium, the original extrema finder is able to find the frequency that violates the passivity. Is the fix to sample more frequency points around this region?

Also I wonder if it's better to merge to pre/2.8 to avoid resolving lots of conflicts?

[caseyflex]

Nice work! When I tried Alec's medium, the original extrema finder is able to find the frequency that violates the passivity. Is the fix to sample more frequency points around this region?

Also I wonder if it's better to merge to pre/2.8 to avoid resolving lots of conflicts?

So originally, I tried using logic as in imag_ep_extrema_with_samples, using an additional frequency range based on the data frequency range. However, the fitter was able to come up with a model that was passive at these additional frequencies too, while still not being uniformly passive, even in the frequency range of interest. So I concluded that the sampling was not working well because of these high-Q poles. The new samples I added should be tailored to the poles themselves, so as to avoid this pitfall of uniform logspace sampling.

Regarding pre/2.8 or develop: technically this is a bug fix, so typically it would go in develop. I think either way I'll be the one resolving the conflicts, and since I did both the fitter separation PR and this one, I should be able to do it without much difficulty either way.

[weiliangjin2021]

So originally, I tried using logic as in imag_ep_extrema_with_samples, using an additional frequency range based on the data frequency range. However, the fitter was able to come up with a model that was passive at these additional frequencies too, while still not being uniformly passive, even in the frequency range of interest. So I concluded that the sampling was not working well because of these high-Q poles. The new samples I added should be tailored to the poles themselves, so as to avoid this pitfall of uniform logspace sampling.

What a smart fitter!

Regarding pre/2.8 or develop: technically this is a bug fix, so typically it would go in develop. I think either way I'll be the one resolving the conflicts, and since I did both the fitter separation PR and this one, I should be able to do it without much difficulty either way.

Sounds good.

[caseyflex]

I checked against most of the material_library. It didn't seem to change most of the fits at all; it changed the following fits in a very small / insignificant way:

AlAs/FernOnton1971
Si3N4/Luke2015PMLStable
Si3N4/Luke2015Sellmeier

so it's possible that the fitter was previously returning mediums that slightly violated passivity at some frequencies for these materials; but anyway it's ok, we aren't using the fitter result in the material library.

And this confirms that the fitter still works well with this change, so we should be OK to merge this.