qiskit
qiskit copied to clipboard
Add functional option to `circuit_key` primitives util
Summary
Adds optional kwarg to request a purely functional key from the circuit_key
primitives utility. This is necessary to avoid recomputation on circuits which are functionally equivalent but vary in non-functional data (e.g. name, id).
Details and comments
Default behavior is set to False
(i.e. default key is not purely functional) this decision has been made for compatibility with the previous implementation, but I think setting the default to True
would be preferred.
EDIT: default behavior set to True
after discussion (see below).
Thank you for opening a new pull request.
Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.
While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.
One or more of the the following people are requested to review this:
- @Qiskit/terra-core
- @ajavadia
- @ikkoham
- @levbishop
- @t-imamichi
Pull Request Test Coverage Report for Build 3126566913
- 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.1%) to 84.421%
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
---|---|---|---|
qiskit/primitives/utils.py | 4 | 5 | 80.0% |
<!-- | Total: | 4 | 5 |
Totals | |
---|---|
Change from base Build 3126106140: | 0.1% |
Covered Lines: | 59599 |
Relevant Lines: | 70597 |
💛 - Coveralls
I believe that id
might cause issues as a matter of fact: if we are mapping the same circuits on the client and server sides, their id
s will differ and so will the keys, since these are assigned by the interpreter and server and client use different interpreters. This subtlety should be considered before committing to it.
I am removing id
from the non-functional key, and setting the default to functional=True
.
id
was kept in the previous PR #8604 because I heard from @ikkoham that @jyu00 and @ikkoham had an agreement to make a composite key in addition to id
. https://github.com/Qiskit/qiskit-terra/pull/8604#issuecomment-1247529117
If they agree to remove id
and name
, I'm fine to remove them. It makes the function simpler.
Yeah we talked about using id
and len(data)
, but this is much better. I agree with removing id
. In fact do we even have a use case for specifying functional=False
?
Thank you, Jessie. We can simplify the code by removing id
and name
. Let's go for it.
IMO, I like to stay on the safe side for these rare cases, in which circuits have different id
and name
but same functionality. As Imamichi-san indicated in the previous PR, collisions are occurring in the corner cases. There are probably collisions that we are not aware and id
and name
might help.
It is a preference which way to fall on the tradeoff between efficiency and safety, so let's take the efficiency side in this case. I believe it is unlikely in normal use and let's hope no bug reports come in 🤞
Okay, let's do the following then:
- We add the
functional
kwarg to allow @ikkoham to include non-functional data when needed. - We default to
functional=True
since this seems to be the best approach to avoid recomputation. - We add
name
to the non-functional key to better handle corner cases. - We get rid of
id
since it may cause conflicts when running different interpreters (e.g. client and server), and when running a single interpreter it stands in it of itself as a unique key: hence making all other values inconsequential in that case.
The PR should now be ready to merge as soon as tests pass 😉
Since this PR is not a new feature, but a follow-up of #8604 to improve efficiency, I would put it in at 0.22.