-
Notifications
You must be signed in to change notification settings - Fork 347
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
Avoid two candidates with same value. #1001
Conversation
Supposed to fix #976 |
num_determinism_failures = 0 | ||
while candidate.get_value_hash() in self._forbidden_value: | ||
num_determinism_failures += 1 | ||
if num_determinism_failures >= 10: |
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.
with this limit we get rid of the risk of infinite / super-long loop.
Still, it would be better to have the auxiliary optimizer (which ensures that we do not go too far) - but Roma was not built in a single day.
@@ -78,7 +78,7 @@ def square2(m: tp.Any) -> float: | |||
model.constraint1 = pyomo.Constraint(rule=lambda m: m.x >= 2) | |||
|
|||
func = core.Pyomo(model) | |||
optimizer = ng.optimizers.OnePlusOne(parametrization=func.parametrization, budget=100) | |||
optimizer = ng.optimizers.OnePlusOne(parametrization=func.parametrization, budget=20) |
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 shows that we are faster than previously.
@@ -146,7 +146,7 @@ def axis(self) -> tp.Optional[tp.Tuple[int, ...]]: | |||
def _apply_array(self, arrays: tp.Sequence[np.ndarray]) -> np.ndarray: | |||
assert len(arrays) == 1 | |||
data = arrays[0] | |||
axis = tuple(range(data.dim)) if self.axis is None else self.axis | |||
axis = tuple(range(len(data))) if self.axis is None else self.axis |
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 understand why this was not caught earlier...
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.
there are too many side effects, I'll need time to analyze this:
- it's a breaking change (people with noisy function, including using noisy optimizers will by default get deterministic behavior)
- it can explode memory for both long runs and huge parametrization
- it's using the value hash which probably needs a lot of revamp because it is hard to implement (leading to issues when adding new parametrizations), and probably too inefficient.
Using request changes to make sure we do not merge it yet.
Types of changes
Motivation and Context / Related issue
Current concerns that need to be tackled (for discussion/tracking):
How Has This Been Tested (if it applies)
Checklist