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 114 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.
Is it necessary to
sympify
the expression here? Isn't it already of the relevant type?I'm not keen on the name of the function (or its existence at all, to be honest, but looking at the existing transpiler pass, the ship has already sailed on that). Perhaps it could be
to_sympy_expression
- I think we should avoid abbreviations where possible (it'sParameterExpression
notParamExpr
, for example), and while we can't always overhaul old code, it'd be good to try and move in that direction going forwards. We useto_
rather thanget_
elsewhere in Terra, so that's better for consistency. It's slightly confusing to me that we'd call itsympy
when it might not return a sympy object, but I can't think of a better name.Please can you expand the documentation to explain when it returns each type, and put in a note that this should not be used with general Qiskit functionality; it's for interoperability only, and most Qiskit functions will not accept raw
sympy
/symengine
objects.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.
Renamed to 'to_simplify_expression', this function returns the simplified version of the expression.
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.
What simplification is being performed here?
sympify
doesn't simplify anything:If this function is meant to simplify its internals, then it should return
ParameterExpression
. If it's meant to return asympy
expression, it should be called something liketo_sympy_expression
with documentation about the fact that it will return asymengine
type if available, and it shouldn't simplify things - that would be up to the user.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.
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 know this was pre-existing, but it'd be good to make this a test of the entire structure of the circuit, not just an op-count - as I understand it, this test should now return an empty circuit, so we should test that.
(As an aside: it maybe would be good to update the test slightly so it doesn't return a fully empty circuit, because a buggy feature completely wiping out all data in the circuit seems like a plausible failure mode.)
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 was attempting to improve this test by using
numpy.testing.assert_allclose
and passing it thecircuit_out
with bound parameters through theOperator
class fromqiskit.quantum_info
, and ran right into an issue I raised dealing withParameter
s not being removed from the parameter table. Since this is not yet fixed, I do not think there is a good way to solve this currently.Perhaps we could move this point to another Issue?
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 looked more, and this test is vitally important, and this PR breaks it as it is. The output of the test should contain 2 CX gates. If you run this test with the changes in this PR, you get
Note that it's replaced the two CXs with two gates whose parameters don't mean anything. This is a failure; the pass should not have replaced anything, even with your changes. If the test had stronger assertions (like I was suggesting in the previous comment), it would have been clearer that this was a true test failure.
Fortunately, now that I understand this, we can hugely simplify the requirements on
UnitaryGate
; we can rewrite the test to avoid it by just using a gate that can handleParameter
in its inverse. Here's a clearer version of the test that's exactly equivalent:If we change the test to that, it lets us revert all the changes to
UnitaryGate
that I now see are unrelated (sorry again about that), and the test is both clearer and stricter.The trick, though, is that I think to achieve your goals in the pass, you're going to need to update your logic a bit. You need to keep track of which parameters are dummy parameters used in the template, and which are parameters that come from the circuit. For example, in this test, the
beta
should not be allowed to be output by the transpiler pass, but in the test you added after this one, thephi
should be output (as you have it).There's more nuance than just that as well, though, because the pass needs to handle the case that a template uses a parameter with the same name as an input circuit without breaking. You can likely handle that bit by re-binding the variables in a template to auto-generated unique names if you detect issues.
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.
That's a great point to eliminate the
UnitaryGate
from the unit test entirely (I implemented the above, and also modifiedtest_parametric_template
similarly). Thetest_unbound_parameters_in_rzx_template
test is now working so thatcircuit_in
andcircuit_out
are verified to not be equal (because the template optimization has changed the circuit), yet the operators are equivalent after bindingphi
to afloat
.In order to guarantee template optimization does not increase the number of
Parameter
s in a circuit, there is a new private method_incr_num_parameters
intemplate_substitution.py
that counts the number ofParameter
s in thetemplate
and in thecircuit
and returns true if the former is greater than the latter. If this is true, logic inside_substitution
will continue out of the method, preventing the template from being substituted.As for
Parameter
s in circuits and templates that have the same name, I'm not sure I know how to auto-generate unique names. Trying this out on the test notebook, all thetemplate
s I use are of therzx
variety, and the specificParameter
used is alwayswhich does not match with
or
so it seems that this is not much of a problem currently. (Of course template optimization fails if I use the exact form of
theta
from the template).