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

refactor!: create ParametrizedFunction interface #353

Merged
merged 12 commits into from
Nov 30, 2021
Merged

Conversation

redeboer
Copy link
Member

@redeboer redeboer commented Nov 25, 2021

The existing Function interface has too many responsibilities: it (1) converts a DataSample to an array or tensor and (2) distinguishes parameter variables from domain variables. This worked fine so far, because Function was only used when optimizing, where you need to tweak those parameters. In #345 though, we need to a Function that only does (1), for the conversion of one DataSample (four-momenta) to another DataSample (kinematic variables) with different back-ends.

This PR creates a new ParametrizedFunction interface that does (1) and (2) and a 'bare' Function interface that does only (1). The input and output types of the Function interface are generic, so that it can be used in different types of function-like interfaces.

Related changes:

  • Renamed LambdifiedFunction to ParametrizedBackendFunction.
  • DataTransformer.transform() has become a __call__ and DataTransformer derives from Function.
  • Estimator also derives from Function: it computes an estimator value for a set of parameters.

@redeboer redeboer added the ⚠️ Interface Breaking changes to the API label Nov 25, 2021
@redeboer redeboer added this to the 0.4.0 milestone Nov 25, 2021
@redeboer redeboer self-assigned this Nov 25, 2021
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@redeboer
Copy link
Member Author

redeboer commented Nov 25, 2021

@spflueger as discussed, ParametrizedFunction inherits from Function:

class ParametrizedFunction(Function):

This is probably fine with interfaces, but another option would be to give ParametrizedFunction a function attribute. This makes sense if Function were to have more attributes (for instance something like expected_keys, for the input DataSample that its call takes). In that case, composition in the interface of ParametrizedFunction may be safer.

Copy link
Member

@spflueger spflueger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good & good job. Just two questions about some design choices

src/tensorwaves/model/function.py Outdated Show resolved Hide resolved
src/tensorwaves/model/sympy.py Outdated Show resolved Hide resolved
src/tensorwaves/model/sympy.py Outdated Show resolved Hide resolved
@redeboer
Copy link
Member Author

redeboer commented Nov 30, 2021

Short summary of offline discussion with @spflueger :

@redeboer redeboer marked this pull request as draft November 30, 2021 16:00
@redeboer redeboer force-pushed the ParametrizedFunction branch 2 times, most recently from 82ce0d4 to bc96b6e Compare November 30, 2021 16:30
@redeboer redeboer marked this pull request as ready for review November 30, 2021 20:45
@redeboer redeboer merged commit 10b5b4f into main Nov 30, 2021
@redeboer redeboer deleted the ParametrizedFunction branch November 30, 2021 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ Interface Breaking changes to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants