MNT: improved check_dimension type inference, added warnings, tests #1067
Conversation
|
Hello @QuentinSoubeyran! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-10-18 20:30:55 UTC |
6fe0e96 to
55cefce
Compare
skopt/space/space.py
Outdated
| def _check_dimension(dimension, transform=None): | ||
| if isinstance(dimension, Dimension): | ||
| return dimension | ||
| elif isinstance(dimension, (list, np.ndarray)): |
There was a problem hiding this comment.
I don't like that a tuple of strings is no longer supported. Why does it need to be a list?
There was a problem hiding this comment.
The idea was to tighten the interface to prevent user errors leading to valid, but unwanted, dimension objects. This is most surely inspired by my uses of type-checkers in python, as the above makes the signature of check_dimension not depend on the actual values contained in the passed sequence:
listbecomes aCategoricaltuple[int, int](and the 3- and 4-long forms) becomeIntegertuple[int, float],tuple[float, int]andtuple[float, float](and corresponding 3 and 4-long forms) becomeReal
It was also to avoid an incorrect tuple silently becoming a Categorical, e.g.
(0, 1, "uniform", base)wherebase="10"was no parsed from a data source, silently returning aCategoricalinstead of the intendedInteger(0, 1, *params)whereparams=(5, "log-uniform", 2)because the semantic/content ofparamsis wrong, silently falling back to aCategorical[True, False]being inferred to anInteger(though the old implementation has a dedicated branch to avoid that).
We could make it so that tuples that fail the Integer/Real inference default to a Categorical as before. This would also allow to use any Iterable, like so:
if isinstance(dimension, tuple) and 2 <= len(dimension) <= 4:
low, high, *args = dimension
if (not args or isinstance(args[0], str)) and (len(args) < 2 or isinstance(args[1], int)):
if isinstance(low, numbers.Integral) and isinstance(high, numbers.Integral):
return Integer(...)
elif isinstance(low, numbers.Real) and isinstance(high, numbers.Real):
return Real(...)
# not an else, fallback
if isinstance(dimension, Iterable):
return Categorical(...)
raise ValueError(...)Do you prefer that other type-tightness ?
skopt/space/space.py
Outdated
| if (isinstance(low, numbers.Integral) | ||
| and isinstance(high, numbers.Integral)): | ||
| return Integer(int(low), int(high), *args, transform=transform) | ||
| elif isinstance(low, numbers.Real) and isinstance(high, numbers.Real): | ||
| return Real(float(low), float(high), *args, transform=transform) |
There was a problem hiding this comment.
low and high are already int (or float). Why cast?
There was a problem hiding this comment.
The float cast is because one of the boundaries might be an int, and a Real dimension asks for two float in its doc.
The int cast was to support other integral types like bool or np.int32.
Since int is not a sublcass of float, I think the float cast is necessary to respect Real's interface. I'm not too sure about the int cast, I mostly did it for symmetry.
| elif all(isinstance(dim, numbers.Integral) for dim in dimension): | ||
| return Integer(*map(int, dimension), transform=transform) |
ac9d5de to
fde2119
Compare
|
@kernc I made tuples fall back to |
Fixes:
This PR fixes
skopt.space.check_dimensionto actually follow its own documentation:listalways lead toCategoricalobjecttuplelead toIntegerorRealobject, as inferred by the bound typescheck_dimensionis retained, and a warning is raised if the improved tightened inference would be different or fail.The new behaviour would fix:
intandfloatbounds may lead to an Integer object storing a float in one of its bounds, leading to crashes when generated points fall outside the float bound due to roundingints but meant aRealobject