-
Notifications
You must be signed in to change notification settings - Fork 42
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
TREGO using batch trust regions #778
Conversation
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.
this is quite neat! surprisingly small amount of changes :)
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 this is great, but if we eliminate TREGO, this would break backward compatibility, right?
I think we should still do it...
@vpicheny ? @uri-granta ?
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.
We don't need to be back-compatible so it's fine to use TREGOBox
f8b46e9
to
8a2cba9
Compare
Related issue(s)/PRs: #773
Summary
This PR adds a TREGO implementation based on the new batch-trust-region classes and replaces the original TREGO
TrustRegion
implementation.Existing notebook, unit tests and integration tests have been updated to use the new classes.
Fully backwards compatible: no
We have agreed that we don't need backwards compatibility -- see discussion below.
Previously, a
TREGO
rule was created like this:This will need to be replaced with the following:
PR checklist