Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow
ParameterExpression
values inTemplateOptimization
pass #6899Allow
ParameterExpression
values inTemplateOptimization
pass #6899Changes from 34 commits
32ee4db
8cfc6d9
c063402
1d290a4
ad85838
7098e4f
3e23380
6e18a54
c7605d7
ed74ccf
e308b84
8fc1fa6
78d3339
cc1a55a
732e887
0c63ca2
6a18dd1
133171c
caee274
bd67056
38e8bde
a99726a
59009db
432bb3f
b308587
5fd736a
71aa17e
5c3a62b
9f373e9
bf590d3
0f5f0d6
4f5b69e
bd3f9ed
6c9831c
d9530e6
06b9cf1
a9e7cdf
4ea8ff5
443a6c9
04dfc37
168c3cc
0e9c39f
d7b55b3
0547f53
fa51c3c
d4044a0
19007b7
d6e1cd7
2065826
a8b9b05
72990f5
d177d84
bd6dda4
6ea9d90
592963b
cebce74
4e2291c
6ccf293
8b6bf37
fdffda6
7721f23
9130a2a
5e31398
eb8ba0d
a92b942
84d65df
17d5002
3db29f0
31b3d62
1e8b009
bdee233
6d419de
b362089
2973339
bb1c797
d531d95
30706cd
9260cf4
767ccf4
28b384f
d55e75c
f50c4e6
e32d038
c21fdf1
bc42afb
cce7b65
5506200
708bfcc
84d566b
54403b0
3c15da0
9355893
7e9f8d9
0a179b6
5f86049
0835763
680ec53
f276cd7
a295831
2f244bc
0434a70
055832d
2504c2e
0f1cf5d
c672ee3
9a94bb2
48173c1
24097b5
2f0203c
591763a
4dcf014
8d1bc2e
4f76b73
c8a08e9
ee081e3
3f740fb
69b8472
58c8ccf
e09b1e3
f01a671
78a2f47
8496a8c
82470fd
1f17aff
fb8ca5c
0ce2fa1
9f59e06
5de0384
7906036
092f92b
95dbcfb
908f87d
fc83156
a14dc37
9d07849
7034789
16bbffb
317e2a6
6ccc663
94e1884
87e5fe8
df9c3d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
for key, value in fake_bind.items():
, but also, isn't this going to add a whole lot of partially bound parameters to thebound_params
list? Iffake_bind
has more than one item in it, this is going to be called a whole lot of times, and there'll be more items inbound_params
than there originally were innode.op.params
.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 think the issue here is that we cannot pass
bound_params
to.bind()
because of the requirement the bound values are numeric, whereas we cannot passbound_params
directly to.assign()
because it takesparameter
andvalue
separately as arguments. Any obvious solution seems to require modification of how.bind()
or.assign()
work.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 don't think this comment matches up with my issue (also the point about iterating through a dictionary with
.items()
still stands). I think this code is buggy, and it's just luckily not triggered.The problem is that if
fake_bind
hasn
elements in it, the length ofbound_params
would ben * len(params)
, which is incorrect. Logically, this code is trying takeparam
, and bind all instances ofParameter
within it to a corresponding value infake_bind
, and return a single value, but instead it adds lots of values tobound_params
. This only works because the templates you're considering only have a singleParameter
in them. If we had a template that had two parameters in it, this would be broken. It's quite possible it was broken before as well, but this breaks it in a new way.Please can you add a test case that involves a template (arbitrary - it can just be invented for the test) that has two parameters, and test that it binds correctly to input circuits with either numeric values or parameter values? For example, here's a (nonsensical) template match that fails with a
KeyError
, even though it should bind just fine:There are two issues at play: one is the issue with the length of
bound_params
, which is new in this PR, and the other is that the calls toParameterExpression.assign
during the template match should assign all parameters in that expression in one go, and only those parameters (this issue I think was pre-existing).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.
If it really isn't possible to have it work for multiple parameters in the template circuit, then this should at least enforce that the template circuit only has a single unbound parameter in it, and the pass should loudly fail if it's given a template with more than one parameter.