-
Notifications
You must be signed in to change notification settings - Fork 131
Changing the way default bounds are handled in RB #369
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
Conversation
yaelbh
left a comment
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 think it's correct, but it will help if @nkanazawa1989 also takes a look
nkanazawa1989
left a comment
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.
Thanks, this looks correct to me.
nkanazawa1989
left a comment
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.
Sorry, I misunderstood the issue.
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.
It seems like the issue and PR are outdated by #390. Now the bounds and init guesses are set via set_if_empty method. Usually the default values entering to the user options are None and actual numbers are provided in the guess method (this is because some bounds are also computed based on the curve data, IRB and RB is just a special case that we know these numbers without having actual data). I think what is implemented now is correct behavior and we don't need to change it.
If user intentionally provides these bounds or guesses, that values are passed to the user options dict and the dict protects itself from updating by algorithmic bounds or guesses.
|
@gadial Do you agree with @nkanazawa1989 ? Should we close this? |
|
Agreed. |
Summary
This PR addresses issue #103.
Details and comments