Fast Dispersion Fitter: keep poles slightly away from input freqs #1672
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tylerflex
approved these changes
May 3, 2024
| for omega in self.omega | ||
| for pole in self.complex_poles | ||
| ): | ||
| return self |
There was a problem hiding this comment.
do we want to warn or anything here?
There was a problem hiding this comment.
thanks. @weiliangjin2021 , do you think we should warn here? I actually think the resulting model in these cases is as expected / as desired, so no need to warn the user. The fix is just preventing the fitter from trying to further improve a model which is already quite good, when the improvement would limit in this error. But I'm not sure exactly about your use case.
There was a problem hiding this comment.
I could also use fp_eps instead of 1e-10. Might be more robust that way.
There was a problem hiding this comment.
Yeah, since it's accurate enough, I think no need to warn.
weiliangjin2021
approved these changes
May 3, 2024
|
This can be merged right? |
|
Yes!
…On Wed, May 8, 2024 at 7:06 PM momchil-flex ***@***.***> wrote:
This can be merged right?
—
Reply to this email directly, view it on GitHub
<#1672 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3KLECNYOY255MGXN33XVTDZBJSTNAVCNFSM6AAAAABHFN2AMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBRGEZDMMZSGQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes in the fitter, the poles move close to the input freqs. When they move too close, the response function is invalid and the fitter errors. This PR keeps them a certain distance (1e-10 rad/s) away.
This PR should fix this issue:
#1631