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.

Sign up for GitHub

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

Jump to bottom

Fix ParamResolver.param_dict type #2550

Merged
merged 3 commits into from
Nov 14, 2019
Merged

Fix ParamResolver.param_dict type #2550

merged 3 commits into from
Nov 14, 2019

Conversation

kevinsung
Copy link
Collaborator

Is this correct? The new type allows keys that are sympy.Basic, not just sympy.Symbol.

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Nov 14, 2019
@cduck
Copy link
Collaborator

cduck commented Nov 14, 2019

I think your change is correct but I'm not sure about the value of ParamDictType = Dict[Union[str, sympy.Basic], Union[float, str, sympy.Symbol]]. I would expect it to be Dict[Union[str, sympy.Symbol], Union[float, str, sympy.Basic]] (map symbols or symbol names to values, symbol names, or arbitrary expressions).

@kevinsung
Copy link
Collaborator Author

Yes, I think you are correct. I've changed it.

cduck
cduck approved these changes Nov 14, 2019
View reviewed changes
Copy link
Collaborator

@cduck cduck left a comment

Choose a reason for hiding this comment

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

LGTM.

@kevinsung kevinsung mentioned this pull request Nov 14, 2019
Closed
@kevinsung kevinsung merged commit 23ce7ca into quantumlib:master Nov 14, 2019
@kevinsung kevinsung deleted the resolver_type branch November 14, 2019 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cduck cduck cduck approved these changes

@Strilanc Strilanc Awaiting requested review from Strilanc

Assignees
No one assigned
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kevinsung @cduck @googlebot