Add uuid property to Parameter

[wshanks]

This change adds a public, read-only uuid property to Parameter which can be used in advanced use cases for getting the uuid value to pass to the Parameter constructor.

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[wshanks]

I tried building the docs locally but I didn't see my release note in the notes. Is there a trick to it? I am never sure if I have written valid sphinx restructured text without compiling it.

A couple questions:

• Add a test?
• Modify the qpy code that accesses _uuid to access uuid?

[mtreinish]

Overall this LGTM, it makes sense to enable a read only attribute for the uuid since it's already a documented part of the constructor. Just one small inline comment/question about the docstring.

[mtreinish]

I tried building the docs locally but I didn't see my release note in the notes. Is there a trick to it? I am never sure if I have written valid sphinx restructured text without compiling it.

A couple questions:

* Add a test?

A test wouldn't hurt but this is also so simple I'm not going to say it's required.

* Modify the qpy code that accesses `_uuid` to access `uuid`?

This is a good idea, and would actually take care of the test coverage for you so you definitely don't need a bespoke test for this attribute.

[mtreinish]

I tagged this for 1.1.0 because it is technically a new feature past the proposal deadline for 1.0.0, but I think it's simple enough to grant an exception to the freeze deadline, but I'll wait for someone else to confirm before re-tagging for 1.0.0

[coveralls]

Pull Request Test Coverage Report for Build 7670104958

• 0 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.04%) to 89.558%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	91.69%

Totals
Change from base Build 7665707920:	0.04%
Covered Lines:	59594
Relevant Lines:	66542

---

💛 - Coveralls

[wshanks]

This is a good idea, and would actually take care of the test coverage for you so you definitely don't need a bespoke test for this attribute.

I pushed a commit that does this. The one thing about it is that other places still refer to _name which I didn't change though there is a name property as well. I don't know if that makes it undesirably inconsistent?

I tagged this for 1.1.0 because it is technically a new feature past the proposal deadline for 1.0.0, but I think it's simple enough to grant an exception to the freeze deadline, but I'll wait for someone else to confirm before re-tagging for 1.0.0

It is not urgent since _uuid can be used in the meantime, but I just wanted to avoid using private attributes if I could.

[jakelishman]

I'm ok on granting a freeze exception to a getter that I think was just an oversight in the interface when the UUID was added in #2947.

Copying in an above comment, because I think it might be in a thread GitHub had already collapsed:

you can't use . suffix matches with intersphinx - it needs to be a fully specified name. In this case, :class:`~uuid.UUID` should do what you want.

[wshanks]

I didn't see a basic test of Parameter._eq__(), so I added one, including a check that using .uuid could produce something that would equal the original instance (I was surprised to learn that the parameter name doesn't matter for equality).

[jakelishman]

I was surprised to learn that the parameter name doesn't matter for equality

Huh, me too, although I suppose that once we've made a UUID, it's wasting time to compare a string as well.

[wshanks]

Huh, me too, although I suppose that once we've made a UUID, it's wasting time to compare a string as well.

Technically, though, Parameter is violating the requirement from the Python data model (which often seems to bite me): "The only required property is that objects which compare equal have the same hash value." Likely, either the name should be included in the hash or the hash key should be just the uuid.

[jakelishman]

Oh, in that case it's a bug yeah thanks - I guess we should add the name to the equality check, since the documentation around the class says they need to have the same name to be equal.

[jakelishman]

Thanks for this, Will. We were already ok with granting this a freeze exception, but I think the discussion in #11654 around how Pulse could use this to more easily adapt to the necessary bugfix of #11652 puts it beyond doubt.