[python] Allow kernel arguments with cudaq.draw's format overload - #5180
Open
udsy19 wants to merge 1 commit into
Open
[python] Allow kernel arguments with cudaq.draw's format overload#5180udsy19 wants to merge 1 commit into
udsy19 wants to merge 1 commit into
Conversation
… arguments
The format-string overload of `cudaq.draw` asserts `len(args) == 1`, but in
that overload `args` is `(kernel, *kernel_args)`, so passing any argument to
the kernel raises `AssertionError`. The very next line, `vargs = args[1:]`,
exists to forward those arguments and is unreachable-by-construction under
`== 1`. The non-format overload forwards `*args` correctly, so the two
overloads disagree, and the documented signature
`cudaq.draw("<format>", kernel, opt_args...)` cannot be used.
This also affects `cudaq.getSVGstring` and `cudaq.displaySVG`, which call
`cudaq.draw("latex", kernel, *args)` in `cudaq/display/display_trace.py`.
Relax the comparison to `>= 1`. Wrong argument counts are still rejected
downstream by `_detail_draw`, which raises `RuntimeError` when the kernel's
formal arity does not match.
Additionally, `assert COND and "msg"` folds the message into the condition,
so the `AssertionError` was raised with no message at all. Use `assert
COND, "msg"` so the "must have a kernel" text is actually reported.
Add a regression test covering the format overload with kernel arguments;
no existing test exercised that combination.
Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
This file contains hidden or 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
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.
The format-string overload of
cudaq.drawcannot be used with a kernel that takesarguments:
In
python/cudaq/runtime/draw.pythe format branch assertedlen(args) == 1, but in thatoverload
argsis(kernel, *kernel_args), so the assertion holds only for a kernel withno arguments. The following line,
vargs = args[1:], exists to forward those arguments andcould never carry a non-empty value. The non-format branch forwards
*argscorrectly, sothe two overloads disagreed with each other, and the signature documented in the function's
own docstring, in the
py_draw.cppdocstring, and in the published API reference was notusable.
cudaq.getSVGstringandcudaq.displaySVGhit the same path —display_trace.pycallscudaq.draw("latex", kernel, *args)— so the SVG renderer was broken for everyparameterized kernel.
This is a regression from #3693, which replaced the C++
drawoverload set with thisPython shim. The previous
pyDraw(std::string format, py::object &kernel, py::args args)forwarded
argsin both itsasciiandlatexbranches.What this changes
Two things on that one line:
==becomes>=, so the assertion only checks what it is named for — that a kernelwas supplied after the format string — and
vargs = args[1:]becomes meaningful.Argument-count validation is not weakened:
_detail_drawalready compares the kernel'sformal arity against the arguments given and raises
RuntimeError: Invalid number of arguments passed to run. N given and M expected.for both too few and too many, in both overloads.
assert COND and "msg"becomesassert COND, "msg". In the first form the string isfolded into the condition — it is truthy, so it never changed the outcome — and the
AssertionErrorwas raised with no message. Callingcudaq.draw("ascii")with nokernel now reports
AssertionError: must have a kernelinstead of a bareAssertionError.A useful control while confirming the diagnosis: the failing script runs correctly under
python -O, which stripsassertstatements and changes nothing else. The rest of thepath already handled the arguments; the assertion was the only thing rejecting them.
Testing
python/tests/visualization/test_draw.pygainstest_draw_format_with_arguments, placednext to
test_draw, which owns the existing zero-argumentcudaq.draw("latex", kernel)case. It checks that the format overload agrees with the non-format overload, that the
parameter value actually reaches the drawing in both
asciiandlatex, and that theno-kernel guard still fires with its message. It fails on
mainatdraw.py:42andpasses with this change.
No existing test covered this combination: the only format-string call sites in the repo
are
test_draw.py:104anddocs/sphinx/examples/python/visualization.ipynb:352, bothwith a zero-argument kernel, while every call site that passes kernel arguments uses the
non-format overload. That is why this survived.
Fixes #5179