Skip to content

Add CachedExpressionConvertible #72

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

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Conversation

braised-babbage
Copy link
Contributor

This adds a new protocol, CachedExpressionConvertible, which can be used analogously to ExpressionConvertible but with support for the constructed expression to be reused during program construction. This can improve performance during construction of programs in which an underlying ExpressionConvertible object be used many times, or when the actual work of AST construction is non-negligible.

Copy link
Collaborator

@PhilReinhold PhilReinhold left a comment

Choose a reason for hiding this comment

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

Aside from a docstring request, LGTM. Handing over to @jcjaskula-aws for braket perspective.

@@ -62,6 +62,7 @@
"ComplexVar",
"DurationVar",
"OQFunctionCall",
"OQIndexExpression",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sneaking in one unrelated change in this PR. I see that previously this was updated (cf 862d0c1), but OQIndexExpression was not actually moved. It should be exported as before.

Copy link
Collaborator

@jcjaskula-aws jcjaskula-aws left a comment

Choose a reason for hiding this comment

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

I don't see any reason right now why this should be a problem for Braket. Would there be any drawback to do this with OQPyExpression instead of creating a new protocol?

@braised-babbage
Copy link
Contributor Author

I don't see any reason right now why this should be a problem for Braket. Would there be any drawback to do this with OQPyExpression instead of creating a new protocol?

I had a vague concern about backwards compatibility here. One could very well imagine somebody constructing a program in which they mutate an ExpressionConvertible object during construction (e.g. in a Python loop over a set of parameters) and rely on Oqpy's current behavior (constructing fresh AST for each usage). I also think that not being able to opt out of this caching behavior could be annoying in certain circumstances.

@PhilReinhold PhilReinhold self-requested a review September 25, 2023 12:56
@PhilReinhold PhilReinhold merged commit 9bee4c7 into main Sep 25, 2023
@jcjaskula-aws jcjaskula-aws deleted the add-cached-expressions branch June 17, 2024 15:56
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

Successfully merging this pull request may close these issues.

3 participants