Add functional option to circuit_key primitives util

[pedrorrivero]

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).

[qiskit-bot]

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

[coveralls]

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%

Totals
Change from base Build 3126106140:	0.1%
Covered Lines:	59599
Relevant Lines:	70597

---

💛 - Coveralls

[ikkoham]

I understood the necessity of the feature. I approve this change, but note that _circuit_key is only a workaround in the first place, which is why it is set to private.

[t-imamichi]

I'm personally fine to remove id from this key. We need to discuss it in the future.

[pedrorrivero]

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 ids 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.

[t-imamichi]

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. #8604 (comment)
If they agree to remove id and name, I'm fine to remove them. It makes the function simpler.

[jyu00]

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?

[t-imamichi]

Thank you, Jessie. We can simplify the code by removing id and name. Let's go for it.

[ikkoham]

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 🤞

[pedrorrivero]

Okay, let's do the following then:

1. We add the functional kwarg to allow @ikkoham to include non-functional data when needed.
2. We default to functional=True since this seems to be the best approach to avoid recomputation.
3. We add name to the non-functional key to better handle corner cases.
4. 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 😉

[t-imamichi]

LGTM

[ikkoham]

Thanks. LGTM.

[ikkoham]

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.