-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added fast dispersion fitter #977
Conversation
some random feedback, not sure if it is already addressed: I happened to play with different fittings recently, and it seemed like |
thanks, should be fixed now |
0e1bfbc
to
a211d5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite good! It now works well with several metals in my tests. Since we start to support gain medium recently, to fit a gain medium, is it by either setting loss_bounds=(-np.inf, np.inf)
or passivity_num_iters=0
? As for the latter, it is currently a PositiveInt, so might be good to change it to an int?
b316e76
to
34000a5
Compare
Thanks for the comments. It's by setting loss_bounds. I improved the docstrings to make this more clear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I don't understand many of the internals so most of my comments are pretty surface level. More generally, are we considering adding more fitters in the future? If so, it would probably be nice to have a more unified interface in terms of parameters, the general .fit()
method (what is displayed, how progress bars work), and the testing. Thanks Casey!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have much to add from my previous comments or Tyler's points. One thing I noticed is that max_num_poles
and min_num_poles
seem very similar in purpose, but are accessible in different parts of the API. Is there any deeper reason for this choice (maybe compatibility with other fitters)?
Other than that, I believe we talked about making this fitter the easy default for the user. Is this change in API supposed to be included in this PR? In that case, it should probably be followed by changes in the docs.
Thanks for the comments. I don't think we are considering adding more fitters in the future. Just keeping the stable fitter as a backup. |
Thanks, changed this so they are both optional arguments now. As discussed, I will change the fitter notebook to showcase this one as the default. |
@weiliangjin2021 @lucas-flexcompute if you both approve of the PR can you formally approve it through GitHub and then I'll merge it. If you want changes, please request them. thanks! |
Features:
Known limitations: