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.

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

Fix UnitaryGate.repeat() method #12986

Merged
merged 14 commits into from
Aug 26, 2024
Merged

Fix UnitaryGate.repeat() method #12986

merged 14 commits into from
Aug 26, 2024

Conversation

timmintam
Copy link
Contributor

@timmintam timmintam commented Aug 19, 2024

Summary

Closes #11990 : error when calling the UnitaryGate.repeat() method (inherited from the Gate class).

Details and comments

The method repeat() internally calls the private method _return_repeat() which is inherited from the Gate class.
It creates a new Gate object with arguments params=self.params, which is expected to be of type ParameterExpression or float. However the parameters are of type numpy.ndarray for a UnitaryGate.
A simple fix is to omit these parameters (params=[]) as they are not directly used by the returned Gate. Actually these parameters are still stored in the repeat() method and used only there.

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 19, 2024
@CLAassistant
Copy link

CLAassistant commented Aug 19, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant

This comment was marked as outdated.

@timmintam timmintam marked this pull request as ready for review August 19, 2024 13:57
@timmintam timmintam requested a review from a team as a code owner August 19, 2024 13:57
@qiskit-bot
Copy link
Collaborator

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 following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks a lot @timmintam! The PR looks pretty good, I just added a small suggestion

releasenotes/notes/fix_11990-8551c7250207fc76.yaml Outdated Show resolved Hide resolved
@Cryoris
Copy link
Contributor

Cryoris commented Aug 19, 2024

This would also happen for a set of other gates that have special params attributes, right? E.g. Isometry (also array), UCGate (list of arrays), PermutationGate (matrix of ints)... If it does, it would be nice to fix this generally 🙂

These would fail because the custom validate_parameter functions are not being used, since _return_repeat constructs a generic Gate. Maybe we could pass that function along?

@1ucian0 1ucian0 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Aug 19, 2024
@timmintam timmintam changed the title Fix/11990/1 Fix UnitaryGate.repeat() medthod Aug 19, 2024
@timmintam timmintam changed the title Fix UnitaryGate.repeat() medthod Fix UnitaryGate.repeat() method Aug 19, 2024
@timmintam
Copy link
Contributor Author

timmintam commented Aug 19, 2024

This would also happen for a set of other gates that have special params attributes, right? E.g. Isometry (also array), UCGate (list of arrays), PermutationGate (matrix of ints)... If it does, it would be nice to fix this generally 🙂

These would fail because the custom validate_parameter functions are not being used, since _return_repeat constructs a generic Gate. Maybe we could pass that function along?

You are right @Cryoris, this would fail for other gates as well actually. Do you mean something along these lines this in the Gate class ?

def _return_repeat(self, exponent: float) -> "Gate":
   gate = Gate(name=f"{self.name}*{exponent}", num_qubits=self.num_qubits, params=[])
   gate.validate_parameter = self.validate_parameter
   gate.params = self.params
   return gate

It seems to do the trick.

@Cryoris
Copy link
Contributor

Cryoris commented Aug 20, 2024

Yeah exactly! 🙂 An alternative solution would be to extend the Gate.__init__ to accept a custom validate_parameter, but in Python we can just override this function and it should be fine.

Could you also add tests for these other gate types that fail?

@coveralls
Copy link

coveralls commented Aug 20, 2024

Pull Request Test Coverage Report for Build 10525187951

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 89.59%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 6 91.73%
Totals Coverage Status
Change from base Build 10525107270: 0.01%
Covered Lines: 67787
Relevant Lines: 75664

💛 - Coveralls

@timmintam timmintam requested a review from ElePT August 21, 2024 08:01
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks a lot timmintam, just to err on the side of caution, could you also add a test for the PermutationGate case in test/python/circuit/library/test_permutation.py?

test/python/circuit/test_isometry.py Outdated Show resolved Hide resolved
ElePT
ElePT previously approved these changes Aug 21, 2024
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Nice! The changes look good to me, but let's wait to see if @Cryoris has any final comment before merging.

fixes:
- |
Fixes an error when calling the method :meth:`.UnitaryGate.repeat`.
Refer to `#11990 <https://github.com/Qiskit/qiskit/issues/11990>` for more
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a trailing underscore (but @ElePT might know this better than me 🙂)

Suggested change
Refer to `#11990 <https://github.com/Qiskit/qiskit/issues/11990>` for more
Refer to `#11990 <https://github.com/Qiskit/qiskit/issues/11990>`__ for more

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the update! Sorry I wasn't explicit about all gates that would need this kind of testing, this should be the complete list:

  • Initialize (though this is an Instruction so this might need the same fix on the Instruction class)
  • StatePreparation
  • DiagonalGate
  • LinearFunction (not called gate but is a gate 🙂 )
  • PauliGate
  • UCPauliRotGate
  • HamiltonianGate
    Could you add a small test for these as well? They can all also be on 1 or 2 qubits max like what you already have 🙂

@timmintam timmintam requested a review from Cryoris August 23, 2024 13:12
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the updates 🙂

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM to me too!

@ElePT ElePT added this pull request to the merge queue Aug 26, 2024
Merged via the queue into Qiskit:main with commit 83ecf39 Aug 26, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Invalid param type <class 'numpy.ndarray'> for gate unitary*1
7 participants