-
Notifications
You must be signed in to change notification settings - Fork 343
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
feat: Expressions without parameters have improved compatibility with numpy. #1757
feat: Expressions without parameters have improved compatibility with numpy. #1757
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
This cast will raise a ValueError if simplification doesn't result in a real number. | ||
""" | ||
value = self._evaluate() | ||
if value.imag != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use np.isclose
with a fairly moderate threshold here? Perhaps 1e-8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd thought about this too but decided it wasn't necessary; if the inputs are all real, imag == 0.0
in all of the inputs and there are no complex operations I can think of that would accumulate a rounding error. So I'd be inclined to leave it unless you have a specific case that would trip it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bramathon Any thoughts on the above? Just want to make sure before I merge as-is.
…contains-expressions-but-not-parameters
…contains-expressions-but-not-parameters
…contains-expressions-but-not-parameters
…contains-expressions-but-not-parameters
Description
Closes #1682.
This implements the numpy array protocol by leaning on quil-rs expression simplification to get a numpy compatible number from the expression, if possible. If not, we still fallback to the default numpy behavior as to not break existing code.
Checklist
master
branch