-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Allow circuits using only SI units to pass through TimeUnitConversion #14027
Conversation
In f0576fa64, the `TimeUnitConversion` pass was modified so that it raises an exception if a circuit has only SI unit delays with no `dt` value set, but the intent seemed to be catch cases where SI units were used alongside instructions with `dt` units. Here the conditions for raising an exception are modified to allow circuits with all SI unit delays (which usually only happens when working with a backend like `AerSimulator`) to pass through. Closes Qiskit#14016
One or more of the following people are relevant to this code:
|
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 think this change makes sense, do you want to add a test to ensure we don't regress here again?
Pull Request Test Coverage Report for Build 13909958997Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
I thought about that but I wasn't sure where the test would go. I didn't see much in the way of existing tests. I think a new |
That is most probably a bug, thanks for finding it. There is already a test that makes sure that SI-only can pass through when But it tests this only for new expression-based delay durations. Perhaps you can add a test beside it for the old-style delay duration specification. |
@@ -126,9 +126,9 @@ def run(self, dag: DAGCircuit): | |||
# Check what units are used in other instructions: dt or SI or mixed | |||
units_other = inst_durations.units_used() | |||
unified_unit = self._unified(units_other) | |||
if unified_unit == "SI" and not has_dt: | |||
if unified_unit in ("SI", "none") and has_si and not has_dt: |
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 think we may just want this:
if (has_si or unified_unit == "SI") and not has_dt:
time_unit = "s"
elif (has_dt or unified_unit == "dt") and not has_si:
time_unit = "dt"
If _unified
returns "dt"
because its input set is empty, then the elif
gets executed, which is the behavior we want (defaulting to dt
).
And wouldn't need the change to return "none"
.
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.
Ugh, there are so many edge cases 🙂 With (has_si or unified_unit == "SI") and not has_dt
is that it would be true for a circuit with all SI unit delays but with instructions in InstructionDurations
that were in dt. Isn't that a problem? (You could also consider the edge case where the instructions with dt units in InstructionDurations
are not used in the current circuit).
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.
Yup, you're right. Would something like this be more readable? This assumes the presence of your addition of returning "none"
in _unified
.
if inst_durations.dt is None:
# Check what units are used in other instructions: dt or SI or mixed
units_other = inst_durations.units_used()
unified_unit = self._unified(units_other)
has_si = has_si or unified_unit in {"SI", "mixed"}
has_dt = has_dt or unified_unit in {"dt", "mixed"}
if has_dt and has_si:
raise TranspilerError(
"Fail to unify time units. SI units "
"and dt unit must not be mixed when dt is not supplied."
)
if has_si:
time_unit = "s"
else:
# has_dt may be false (i.e. 'unified_unit' is "none"), but we still default to 'dt'
# in this case.
time_unit = "dt"
else:
time_unit = "dt"
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.
That looks good to me. I tried pushing a version like that.
@kevinhartman I wrote some tests before I saw your comment about the existing tests. I pushed them for reference. We move or rework them if you think that is better. |
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.
Looking good, just a few minor comments with the testing.
@unpack | ||
def test_gate_and_delay(self, delay_unit, inst_durations_unit, target_type): | ||
"""Test delays are converted, passed through or flagged as appropriate""" | ||
if delay_unit == "s": |
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 think we generally prefer to "unroll" test code so it's simple to debug when something goes wrong. The orange flag for me here is that the test contains logic to special-case itself based on the ddt
inputs. If you don't mind, I think it'd be helpful if you could break this into three separate tests instead, even though it'll be more verbose.
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 moved the tests to test_transpiler in b749029. To fit in there, I used transpile()
instead of the TimeUnitConversion
interface. That shifted the scope of what seemed like relevant tests. TimeUnitConversion
takes instruction durations and a target and behaves differently depending on the units of the InstructionDurations
object and whether or not it or the target have a dt. With transpile
, the durations only come from the backend and that just comes from backend.target.durations()
. In Qiskit, InstructionProperties
specifies duration in seconds so that prevents dt units in the InstructionDurations
. So that removes a lot of the variants I was covering with my tests before. The si->dt case already had coverage so I added the most common cases of si->si with no backend or a backend without dt and si->error with mixed units and no dt. I think the other nuances my previous tests were covering about instruction durations versus target precedence are not so important for transpile
since the input is only backend or target and not instruction durations directly.
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.
Makes sense. Presumably, the tests you've added wouldn't have passed before your change at least, yeah?
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.
test.python.compiler.test_transpiler.TestTranspile.test_delay_converts_to_seconds
fails as expected:
test.python.compiler.test_transpiler.TestTranspile.test_delay_converts_to_seconds
---------------------------------------------------------------------------------
Captured traceback:
~~~~~~~~~~~~~~~~~~~
Traceback (most recent call last):
File "/qiskit-terra/test/python/compiler/test_transpiler.py", line 1382, in test_delay_converts_to_seconds
out = transpile([qc, qc], seed_transpiler=42)
File "/qiskit-terra/qiskit/compiler/transpiler.py", line 291, in transpile
out_circuits = pm.run(circuits, callback=callback, num_processes=num_processes)
...
File "/qiskit-terra/qiskit/transpiler/passes/scheduling/time_unit_conversion.py", line 134, in run
raise TranspilerError(
...<2 lines>...
)
qiskit.transpiler.exceptions.TranspilerError: 'Fail to unify time units. SI units and dt unit must not be mixed when dt is not supplied.'
test_rejects_mixed_units_delay_without_target_dt
passes, but I added that one because I was trying to carry over my ddt tests and didn't see a test like it (there was an expression version) and not because that behavior was affected by my change.
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.
Looks great, thanks a lot for the fix!
If it doesn't pass lint, I'll come back to approve again after you update.
@@ -90,6 +90,7 @@ | |||
from qiskit.transpiler.passmanager_config import PassManagerConfig | |||
from qiskit.transpiler.preset_passmanagers import generate_preset_pass_manager, level_0_pass_manager | |||
from qiskit.transpiler.target import ( | |||
InstructionDurations, |
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.
Lint is likely to yell at you here 🙂.
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.
Oh, right, this was from when I was trying to cover InstructionDurations
in the old test before I realized that it was not really directly relevant for transpile
since InstructionProperties
is in seconds and transpile
does not take instruction_durations
as an input.
Oops, there was a bad variable name left in from when I was debugging and some stray changes in tox.ini which I have removed now. Hopefully the tests are clean now. (I did lint locally but just missed the warnings. I don't have a smooth setup for Qiskit right now which is part of why I was fiddling with tox.ini. I was getting extra linting warnings possibly because of some other pylintrc file I have or something like that). |
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.
Thanks for the updates!
This looks good to me now.
…#14027) * Allow circuits using only SI units to pass through TimeUnitConversion In f0576fa64, the `TimeUnitConversion` pass was modified so that it raises an exception if a circuit has only SI unit delays with no `dt` value set, but the intent seemed to be catch cases where SI units were used alongside instructions with `dt` units. Here the conditions for raising an exception are modified to allow circuits with all SI unit delays (which usually only happens when working with a backend like `AerSimulator`) to pass through. Closes #14016 * Add TimeUnitConverion tests * Update test description * Remove comment * lint test file * Refactor TimeUnitConversion for readability * Move time unit converison tests to test_transpiler * lint * lint (cherry picked from commit 740a9dc)
In f0576fa64, the
TimeUnitConversion
pass was modified so that it raises an exception if a circuit has only SI unit delays with nodt
value set, but the intent seemed to be catch cases where SI units were used alongside instructions withdt
units. Here the conditions for raising an exception are modified to allow circuits with all SI unit delays (which usually only happens when working with a backend likeAerSimulator
) to pass through.Closes #14016