Make circuit drawers *not crash* on Expr nodes
#10504
Conversation
This at least causes the circuit visualisers to not crash when encountering an `Expr` node, and instead emit a warning and make a best-effort attempt (except for LaTeX) to output _something_. We intend to extend the capabilities of these drawers in the future.
jakelishman
added
stable backport potential
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5681519764
💛 - Coveralls |
There was a problem hiding this comment.
Overall this LGTM, it seems straightforward to just pull the classical bits off the expression and draw the circuit as it would otherwise (with the exception of latex). My only question is about emitting the warning. It seems potentially a bit noisy for users of expression circuits to warn every time the function is called draw function is called on circuits with expr in them.
| @@ -416,7 +417,10 @@ def _build_latex_array(self): | |||
| num_cols_op = 1 | |||
| wire_list = [self._wire_map[qarg] for qarg in node.qargs if qarg in self._qubits] | |||
| if getattr(op, "condition", None): | |||
| self._add_condition(op, wire_list, column) | |||
| if isinstance(op.condition, expr.Expr): | |||
| warn("ignoring expression condition, which is not supported yet") | |||
There was a problem hiding this comment.
Do you think UserWarning is appropriate here? I guess we want people to see this, but I'm wondering if it'll be too noisy? Especially since it's not necessarily user actionable if they're using classical expressions.
There was a problem hiding this comment.
It should only get shown once per drawing, so I think it's ok. It's particularly important to be visible in the LaTeX drawer, because it straight up skips drawing any condition. The text and MPL drawers both make an attempt to show the data dependencies still.
There was a problem hiding this comment.
Maybe just remove it for mpl and text then and leave it in the docstring. For latex I agree it's not correct and we should warn.
|
tbf, if anything, I'd think it's the MPL and text drawers whose warnings might be too noisy, because they still produce sensible (but not necessarily the prettiest) output, so there's still valid cause to use them. The LaTeX drawer produces technically invalid output, so I think the warning's most appropriate. I'm not certain what would be better with the warnings, though - I'm not a big fan of a global "show warning if ..." filter since that takes away users' ability to control things, but maybe it's more appropriate here? |
* Make circuit drawers *not crash* on `Expr` nodes This at least causes the circuit visualisers to not crash when encountering an `Expr` node, and instead emit a warning and make a best-effort attempt (except for LaTeX) to output _something_. We intend to extend the capabilities of these drawers in the future. * Soften warnings about unsupported `Expr` nodes (cherry picked from commit c8552f6)
* Make circuit drawers *not crash* on `Expr` nodes This at least causes the circuit visualisers to not crash when encountering an `Expr` node, and instead emit a warning and make a best-effort attempt (except for LaTeX) to output _something_. We intend to extend the capabilities of these drawers in the future. * Soften warnings about unsupported `Expr` nodes (cherry picked from commit c8552f6) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
* Make circuit drawers *not crash* on `Expr` nodes This at least causes the circuit visualisers to not crash when encountering an `Expr` node, and instead emit a warning and make a best-effort attempt (except for LaTeX) to output _something_. We intend to extend the capabilities of these drawers in the future. * Soften warnings about unsupported `Expr` nodes
Summary
This at least causes the circuit visualisers to not crash when encountering an
Exprnode, and instead emit a warning and make a best-effort attempt (except for LaTeX) to output something. We intend to extend the capabilities of these drawers in the future.Details and comments
This is not an ideal situation, and I've not written any tests really because there's not really much to test here. The intent is just to improve the situation beyond a crash to a warning that something's amiss, and display something. We're hoping to add more complete support later.