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

[python] better decouple TransformationList from search #61

Open
3 of 6 tasks
ftynse opened this issue Nov 29, 2021 · 1 comment
Open
3 of 6 tasks

[python] better decouple TransformationList from search #61

ftynse opened this issue Nov 29, 2021 · 1 comment
Assignees

Comments

@ftynse
Copy link
Contributor

ftynse commented Nov 29, 2021

Currently, the design of the TransformationList class is at least partially driven by search with quite intrusive API changes. Yet it is also intended for manual use. The following changes can make it nicer:

  • Move the Variable class that specifies tunable variables into individual transformations instead of lists and make them compose on creation.
  • Separate TransformationList from TunableTransformationList; the former is basically a list of configured transformation that can be obtained by simple concatenation, the latter is a list of transformations yet to be configured that needs significantly more complex work of creating a unified constructor and a list of tunable variables; Only the tunable lists should be available in "experts.py";
  • Drop the reliance on indiscriminate mapping of constructor kwargs to class attributes in TransformationList;
  • Extend the Variable class for uses beyond search, in particular make some variables "unsearchable" and support default values so they need not be provided when constructing the Transformation or a list thereof;
  • Drop the forwarding of kwargs indiscriminately to all transformations in a list and have an assertion checking for only expected kwargs (otherwise, simple typos make transformations no longer apply, which is sad);
  • Make it possible to partially configure a transformation or a list thereof (possibly related to extending the Variable class).
@shabalind
Copy link
Collaborator

Internal consensus decided to drop the current local_search implementation for the time being. The only coupling that still remains is that we still have variables declarations which are not used for anything concrete any longer. We could probably drop it completely, but it's still used by transformations to make it possible to merge things without name clashes using a.then(b).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants