Moving up some parameter checks in the class hierarchy #3668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1ucian0
requested review from
ajavadia,
ewinston,
kdk and
mtreinish
as code owners
ewinston
reviewed
Jan 7, 2020
kdk
reviewed
Jan 31, 2020
There was a problem hiding this comment.
Looks good, one question is whether or not the base Instruction class should default permissive or default restrictive. Default permissive is less likely to break existing users as it matches previous behavior, but maybe its the case that substantially more gates would have to specify that they e.g. don't accept complex parameters. Alternately, subclasses could be annotated with the types they support. Thoughts?
added 3 commits
April 1, 2020 13:18
mtreinish
reviewed
Apr 1, 2020
There was a problem hiding this comment.
I'm a bit confused here. Like we're moving the type checking for each parameter in params() to a normalize_parameters method so subclasses can override it and define their own behavior, that makes sense to me. But, none of the subclasses look like they do anything different because we call the parent's method if it's not one of the expected types for the subclass.
1ucian0
changed the title
1ucian0
changed the title
added 2 commits
April 7, 2020 18:00
|
57854df adds |
mtreinish
approved these changes
Apr 8, 2020
There was a problem hiding this comment.
Based on a discussion offline we decided that for the base Instruction class they should accept anything and it's on the users to not manually set a parameter type that can't be handled by the whatever the output mechanism of the circuit is. With that open question settled this PR LGTM.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Apr 9, 2020
…t#3668)" This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87c.
mtreinish
added a commit
that referenced
this pull request
Apr 9, 2020
…" (#4117) This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87c.
ajavadia
pushed a commit
to ajavadia/qiskit
that referenced
this pull request
Apr 11, 2020
…t#3668)" (Qiskit#4117) This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87c. add new fake backends
kdk
added a commit
that referenced
this pull request
Apr 17, 2020
…4129) * Revert "Moving up some parameter checks in the class hierarchy (#3668)" (#4117) This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87c. add new fake backends * fix dt and dtm conversion by qiskit to SI * fix imports * lint * Update qiskit/test/mock/backends/armonk/fake_armonk.py Co-Authored-By: Kevin Krsulich <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
This was referenced Apr 19, 2020
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
* abstract method * moving some checks up the Class hierarchy * lint * lint! * normalize_parameter * merge * instruction * no need to override in gate * clean qiskit/extensions/quantum_initializer/diag.py diff * new approach * mising import * validate_parameter * skip multiplexer * docstring * lint * deprecation warning * custom gates are gates * add np.number for gate parameters * lint * barrier validate_parameter * syntax error * message * test barrier * no warning * unused import Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
…t#3668)" (Qiskit#4117) This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87c.
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
…iskit#4129) * Revert "Moving up some parameter checks in the class hierarchy (Qiskit#3668)" (Qiskit#4117) This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87c. add new fake backends * fix dt and dtm conversion by qiskit to SI * fix imports * lint * Update qiskit/test/mock/backends/armonk/fake_armonk.py Co-Authored-By: Kevin Krsulich <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT
pushed a commit
to ElePT/qiskit-ibm-provider
that referenced
this pull request
Oct 9, 2023
…iskit/qiskit#4129) * Revert "Moving up some parameter checks in the class hierarchy (Qiskit/qiskit#3668)" (Qiskit/qiskit#4117) This change has some unforseen backwards compatibility implications and is causing downstream projects like qiskit-aer to fail CI now. Given the pending release and this not being critical, this commit reverts this commit and we can revisit it after the 0.13.0 release. This reverts commit 5afe87ce1e5279ca716fdf5a3c478f2e27a6519f. add new fake backends * fix dt and dtm conversion by qiskit to SI * fix imports * lint * Update qiskit/test/mock/backends/armonk/fake_armonk.py Co-Authored-By: Kevin Krsulich <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3282
@nonhermitian notices that parameters are too broadly checked. I moved some of the checks up the class hierarchy. This allows to be more prices in the kind of check and normalization for each class. Also, it can be improved further with a better class hierarchy for
Instructions. But that's story for another time.All the tests are passing. If reviewers find a uncovered case, let's add the test!