Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure that Parameters compare equal when they have the same hash #11652

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Ensure that Parameters compare equal when they have the same hash #11652

merged 2 commits into from
Feb 23, 2024

Conversation

wshanks
Copy link
Contributor

@wshanks wshanks commented Jan 26, 2024 •
edited by jakelishman
Loading

Previously, only the Parameter.uuid was compared, so Paramemter
instances with different names could compare equal if they had been
constructed using a common value for the uuid parameter (which is
usually not passed explicitly).

Fix #11654

@wshanks wshanks requested review from a team as code owners January 26, 2024 19:06
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @ajavadia
  • @ikkoham
  • @levbishop
  • @mtreinish
  • @nkanazawa1989
  • @t-imamichi

@wshanks wshanks added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jan 26, 2024
@wshanks
Copy link
Contributor Author

wshanks commented Jan 26, 2024

Currently, I based this on #11647 so that I could add to the test from there and reference uuid in Sphinx. This can be reworked if #11647 can only go in to 1.1 and this PR is wanted in 1.0 as a bug fix.

@wshanks wshanks marked this pull request as draft January 26, 2024 19:34
@wshanks
Copy link
Contributor Author

wshanks commented Jan 26, 2024

It looks like there are some existing uses of different names comparing equal so this one may take some more careful work.

@wshanks wshanks mentioned this pull request Jan 26, 2024
Closed
@wshanks
Copy link
Contributor Author

wshanks commented Jan 26, 2024

This will wait for a resolution in #11654

@jakelishman jakelishman mentioned this pull request Jan 26, 2024
Merged
jakelishman
jakelishman reviewed Jan 31, 2024
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Will - pending resolution of #11654 this looks good to go, except for maybe there was a slightly dodgy merge somewhere?

qiskit/primitives/containers/bindings_array.py Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what happened here, but I will clean it up. I must have done something funny splitting this change out from the uuid property which I had together originally.

Copy link
Contributor Author

@wshanks wshanks Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I wanted to use this with the changes in #11647 before that one was merged, so this was based on it. That is why it had the two release notes files for example. I am not sure how I got the extra binding array commit in in addition to the #11647 commits.

releasenotes/notes/parameter-uuid-60dd46a739afabce.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter at all, but for the future: you can put more than one bullet (and even more than one section) in a release note.

test/python/primitives/containers/test_bindings_array.py Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad merge?

@jakelishman jakelishman added this to the 1.0.0 milestone Jan 31, 2024
@jakelishman jakelishman added the mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library label Jan 31, 2024
@wshanks
Ensure that Parameters compare equal when they have the same hash
2ad5fda 
Previously, only the `Parameter.uuid` was compared, so `Paramemter`
instances with different names could compare equal if they had been
constructed using a common value for the `uuid` parameter (which is
usually not passed explicitly).
@wshanks
Copy link
Contributor Author

wshanks commented Jan 31, 2024

#11692 modifies the tests that are currently failing here so that they are not affected by this change.

@jakelishman jakelishman mentioned this pull request Jan 31, 2024
Merged
@jakelishman
Copy link
Member

In the interests of time, and because this PR has an extended critical path to merge, I'm going to defer it to 1.0.1. It's a bugfix - the current definition of equality is broken and not what's documented or compatible with the hash.

@jakelishman jakelishman modified the milestones: 1.0.0, 1.0.1 Feb 1, 2024
@jakelishman jakelishman modified the milestones: 1.0.1, 1.0.2 Feb 22, 2024
@wshanks wshanks marked this pull request as ready for review February 23, 2024 16:21
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8022059400

Details

  • 0 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.008%) to 89.285%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.81%
crates/qasm2/src/lex.rs 5 91.18%
Totals Coverage Status
Change from base Build 8019880275: -0.008%
Covered Lines: 58897
Relevant Lines: 65965
💛 - Coveralls

jakelishman
jakelishman approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Will, this looks good to me, and it's great to fix a data-model problem like this.

@jakelishman jakelishman added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Feb 23, 2024
ihincks
ihincks approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wshanks

@jakelishman jakelishman added this pull request to the merge queue Feb 23, 2024
Merged via the queue into Qiskit:main with commit 3e6b646 Feb 23, 2024
12 checks passed
mergify bot pushed a commit that referenced this pull request Feb 23, 2024
@wshanks @mergify
Ensure that Parameters compare equal when they have the same hash (#1…
46f2de0 
…1652)

Previously, only the `Parameter.uuid` was compared, so `Paramemter`
instances with different names could compare equal if they had been
constructed using a common value for the `uuid` parameter (which is
usually not passed explicitly).

(cherry picked from commit 3e6b646)
@mergify mergify bot mentioned this pull request Feb 23, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 23, 2024
@mergify @wshanks
Ensure that Parameters compare equal when they have the same hash (#1…
887d095 
…1652) (#11875)

Previously, only the `Parameter.uuid` was compared, so `Paramemter`
instances with different names could compare equal if they had been
constructed using a common value for the `uuid` parameter (which is
usually not passed explicitly).

(cherry picked from commit 3e6b646)

Co-authored-by: Will Shanks <[email protected]>
@wshanks wshanks deleted the parameter-hash-eq branch April 5, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

@ihincks ihincks ihincks approved these changes

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
1.0.2
Development

Successfully merging this pull request may close these issues.

Parameter instances can compare equal while having different hash values
5 participants
@wshanks @qiskit-bot @jakelishman @coveralls @ihincks