-
Notifications
You must be signed in to change notification settings - Fork 160
Update twoQ_clifford_error
function
#368
Update twoQ_clifford_error
function
#368
Conversation
I added both
which have interfaces conform to
As @ShellyGarion mentioned in #367, |
added deprecation warning to the old |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Couple of non-blocking comments if you would like to change, and a question
warn('The function `count_gates` will be deprecated. ' | ||
'Gate count is integrated into `gates_per_clifford` function.', | ||
DeprecationWarning) | ||
warn('The function `count_gates` will be deprecated. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to break the string over multiple lines like this was previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed signature of QiskitError
is *message
and we can feed multiple lines, but that of warnings is message, category=None, stacklevel=1, source=None
. for warnings we need to feed one long message with \
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually the case, @chriseclectic is correct the \
here is not necessary and I'm actually surprised that one of the linters didn't complain about it. You can break the string over multiple lines as long as it's inside parenthesis. It will automatically concatenate any strings until the first ,
which indicates the next argument. This is pretty easy to test in the Python REPL too if you want to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtreinish Thanks for following up. I ran local tox -epy36 test and it caused an error due to this (as far as I remember that was a type error). I tried to reproduce this but the test passed after git clean -fdX. Maybe something odd was happening in my local env. If you want I'll make another PR to remove \
.
…nazawa1989/qiskit-ignis into issue-367_update_twoQ_clifford_error
@chriseclectic thanks for reviewing. updated and replied to your questions. |
Summary
fix #367
Details and comments