Skip to content
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

Suggestion: Refactoring of Transforms #888

Open
lebrice opened this issue Apr 14, 2022 · 2 comments
Open

Suggestion: Refactoring of Transforms #888

lebrice opened this issue Apr 14, 2022 · 2 comments
Labels
enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt)

Comments

@lebrice
Copy link
Collaborator

lebrice commented Apr 14, 2022

I have an idea which I believe could greatly simplify a lot of code related to spaces, transforms, and such.

Here is the current state of things:

  • TransformedDimension is a wrapper around a dimension, and changes the value of the type attribute.
  • Transformations are implemented using the Transformer class.
  • This transformer class has a domain_type and target_type class attribute that indicates the input/output type.
  • The input/output types of the transforms are used by the build_required_space function, which gets a Transformer that can map from input to output, and passes it to a new TransformedDimension.

Here is what I suggest we do instead:

  • Remove TransformedDimension
  • Remove TransformedSpace
  • Move the transform and reverse attributes into the Dimension class, and make them optional, with default of None.
  • Refactor the transforms into functions that map from one Dimension to another Dimension.

For example:

@register_transform
def Quantize(dim: Real) -> Integer:
    """Transform real numbers to integers. Isn't perfectly reversible."""
    # todo: create an Integer dimension with the right values.
    lower = quantize(dim.lower)
    upper = quantize(dim.upper)
    out = Integer(name=dim.name, prior=dim.prior, lower=lower, upper=upper)
    out.transform = quantize
    out.reverse = reverse_quantize
    return out

Where the register_transform decorator just registers the function into a dictionary that maps from (in_type, out_type) -> callable, using the type annotations of the function.

The build_required_space would then be a search over the transforms and transform chains, to find one that maps from input to outputs, then applying those to the dimensions, and returning a regular Space with those transformed dimensions.

I'lll add more ideas soon.

@lebrice lebrice added bug Indicates an unexpected problem or unintended behavior enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt) and removed bug Indicates an unexpected problem or unintended behavior labels Apr 14, 2022
@bouthilx
Copy link
Member

I think that would be a great way of simplifying the maintenance of the transformations, as we would not need to reimplement all dimension methods to wrap them (like prior_string, cardinality, interval, etc). There could be a major issue with the priors however. If we decide to stick with a small subset of priors like uniform and loguniform it would be fine. We know how to transform them. If we use priors from scipy.stats however it's not clear at all how we could convert them. It could be fine if the transformations are simply turning Real to Integer as we can reuse the prior as is. The opposite would not be possible however. Say we have Integer(prior=uniform(0, 10)) and convert to Real, we should have Real(prior=randint(0, 10)), not Real(prior=uniform(0, 10)).

That being said, supporting all of scipy.stats has not proved useful at all so far, so departing from it in order to simplify the implementation of the transformations may be a good idea.

@bouthilx
Copy link
Member

I'm revisiting this in order to solve handling of transformed spaces in #832. I think the simplest solution may be to keep the TransformedDimension and TransformedSpace, but only use them for transforming spaces and points instead of behaving like space and dimensions objects.

transformer = SpaceTransformer(original_space, requirements)
assert transformer.original == original_space
transformed_space = transformer.target

transformed_point = transformer.transform(original_space.sample()[0])
original_point = transformer.reverse(transformed_point)

One reason we cannot dispatch transform/reverse on Dimension class only is that some transformations are across dimensions (namely Reshape). The main difference between this proposed solution with current implementation is that transformer.target is a standalone Space object which can be passed to the algorithm instead of the TransformedSpace that is currently passed. This would allow removing many of the methods of TransformedDimension subclasses as it would not need anymore to support things like prior_string, interval, cardinality, as they would be defined in base Space object at transformer.target. Having a base Space object representing the transformed space will make it much easier to implement conversions on transformed spaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt)
Projects
None yet
Development

No branches or pull requests

2 participants