Conversation
AVHopp
left a comment
There was a problem hiding this comment.
Like the way this is going! Have some more ideas for further simplification
AVHopp
left a comment
There was a problem hiding this comment.
Some comments related to parts of the design that I still do not fully agree with resp. understand
|
@Hrovatin any movement in this PR or should we close it? |
|
@Scienfitz still on my todo list to add your last two comments |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Updated the docstring to clarify that the callable now shifts both dimensions and bounds for the Hartmann function.
9efb03a to
2547819
Compare
Scienfitz
left a comment
There was a problem hiding this comment.
lgtm, last comments but otherwise ready for merge
benchmarks/domains/hartmann/utils.py
Outdated
| raise ValueError("Shift shape does not match used dimensions.") | ||
| self.shift = shift if shift is not None else [0.0] * bounds_array.shape[0] | ||
|
|
||
| # Shift the bounds |
There was a problem hiding this comment.
| # Shift the bounds | |
| # Extend the bounds |
"shift" would only be accurate if you always change lower and upper end by same amount
Actually, this bounds = expression below still looks overly complex to me, the expression reads terribly. Naively one would shift both upper and lower bound by the same amount, that doesn't work?
Assuming there is a reason for that, I can still think of this:
bounds = [
(low - abs(s), high + abs(s))
for (low, high), s in zip(bounds_array, self.shift)
](or similar)
which is not shifting but widening the bounds (should also work for passing the validation and is a more readable expression).
There was a problem hiding this comment.
I needed to adapt it as follows
# Note: We can not only shift the upper and lower bounds as that leads to the
# optimal value (``_optimizer``) being excluded from bounds. As this is
# hard-coded in the Hartmann class init, we can not override it before it is
# evaluated.
shifted_bounds = bounds + np.array(self.shift)[:, None]
bounds_extended = list(
map(
tuple,
np.stack(
[
np.minimum(bounds[:, 0], shifted_bounds[:, 0]),
np.maximum(bounds[:, 1], shifted_bounds[:, 1]),
],
axis=1,
),
)
)
There was a problem hiding this comment.
does the benchmark work when running locally?
There was a problem hiding this comment.
For me it did with python -m benchmarks ...
Add two new TL benchmarks:
@kalama-ai