Custom boolean functions for supporting noise models #5674
Conversation
|
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5674 +/- ##
==========================================
- Coverage 99.68% 99.67% -0.01%
==========================================
Files 415 417 +2
Lines 38898 38748 -150
==========================================
- Hits 38774 38623 -151
- Misses 124 125 +1 ☔ View full report in Codecov by Sentry. |
|
Thanks @obliviateandsurrender! Would you mind linking the corresponding SC story? |
There was a problem hiding this comment.
Thanks @obliviateandsurrender! Will look again at the docs as this PR evolves.
…lane into noise-models-1
| Conditional Constructors | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| .. currentmodule:: pennylane.noise.conditionals | ||
|
|
||
| .. autosummary:: | ||
| :toctree: api | ||
|
|
||
| ~op_eq | ||
| ~op_in | ||
| ~wires_eq | ||
| ~wires_in |
There was a problem hiding this comment.
It might be worth mentioning BooleanFn here, both because it is the base of all of these but more importantly because it can also be used for custom conditionals.
pennylane/boolean_fn.py
Outdated
| @property | ||
| def bitwise(self): | ||
| """Determine whether wrapped callable is for a bitwise operation or not.""" | ||
| return bool(getattr(self, "operands", tuple())) |
There was a problem hiding this comment.
So this property is for And, Or, etc.?
There was a problem hiding this comment.
Yep! This will help us know if the given instance is a result of a bitwise operation.
There was a problem hiding this comment.
🤔 any reason why not to use instance checks for this?
There was a problem hiding this comment.
We could do instance checks as well. I find this to be more generalized as it makes this property robust to any bitwise class that is added next or is user-made.
pennylane/boolean_fn.py
Outdated
| @property | ||
| def conditional(self): | ||
| """Determine whether wrapped callable is for a conditional or not.""" | ||
| return bool(getattr(self, "condition", None)) |
There was a problem hiding this comment.
And this property is for the OpEq, etc., noise-specific conditionals?
There was a problem hiding this comment.
Yep, this is for the conditionals we provide in the noise module.
pennylane/boolean_fn.py
Outdated
| @property | ||
| def bitwise(self): | ||
| """Determine whether wrapped callable is for a bitwise operation or not.""" | ||
| return bool(getattr(self, "operands", tuple())) |
There was a problem hiding this comment.
🤔 any reason why not to use instance checks for this?
| .. seealso:: Users are advised to use :func:`~.wires_eq` for a functional construction. | ||
| """ | ||
|
|
||
| def __init__(self, wires): |
There was a problem hiding this comment.
I might be misremembering, but I thought I saw you show in the meeting the input argument of WiresEq being an operation?
There was a problem hiding this comment.
Oh it's in the wires_in and wires_eq function below 🤔 Why do we need operation input?
There was a problem hiding this comment.
This is just an additional support to make the constructor functions more flexible and easy to use for the users. Especially in a scenario where they are trying to automate building noise models based on some set of operations they have in their pipeline, then they can still make these conditional without having to extract wires manually.
pennylane/noise/conditionals.py
Outdated
| ops (str, Operation, Union(list[str, Operation])): string | ||
| representation or instance of the operation. |
There was a problem hiding this comment.
Here I was expecting op_in([qml.RX, qml.Hadamard]). Is this approach challenging/not advisable?
There was a problem hiding this comment.
I think I just forgot to mention it, but doing that is possible as well! I'll add it to the docs. 😁
| >>> func = qml.noise.partial_wires(qml.RX(1.2, [12])) | ||
| >>> func(2) | ||
| qml.RX(1.2, wires=[2]) |
There was a problem hiding this comment.
I'm confused, so we originally have an instantiated RX on wire 12 but it gets mapped to wire 2?
There was a problem hiding this comment.
Yes. We had an operation that was RX(1.2, wires=12). When we wrap it with a partial_wires, we can instantiate that operation with the new input wires, in this case, 2.
| >>> func = qml.noise.partial_wires(qml.RX(1.2, [12])) | ||
| >>> func(2) | ||
| qml.RX(1.2, wires=[2]) | ||
| >>> func(qml.RY(1.0, ["wires"])) |
There was a problem hiding this comment.
🤔 Do we need to support calling on operations, rather than just wires?
There was a problem hiding this comment.
Looking good so far. I added a few suggestions to clean up the code, along with some clarification questions. The two big questions I have:
- The only use of the bitwise and condition properties so far is to determine what the
reprof the BooleanFunction is going to be. If they are just being used as flags, then I can suggest two alternatives that might be cleaner and more space efficient:
-
Update the base
reprmethod to always returnBooleanFn(self.fn.name), and in each of the sub classes, just overwrite thereprmethod directly (as you have done for thestrmethod. You can also drop thenamekwarg. -
Update the base
reprmethod to returnBooleanFn(self.fn.name)ifname is Noneelse returnname. The only time name is provided is when the sub classes are conditionals or bitwise operators. In the case that a user provides a name, we probably want to display that anyways.
- Regarding
OpEq, it doesn't seem to me that we need it? It's just a special case ofOpInwhen the list of operations is length 1. Also it seems to be coded to allow passing in multiple operators both as the reference list and as the items being compared, but this doesn't make sense to me because when we use this conditional in a circuit, we are going to iterate over the gates in the circuit one at a time anyways? So the item being compared will always be a single operation. Furthermore, comparing multiple lists on both sides can be easily achieved by using the map function, so it's probably not worth it? Happy to discuss offline if I am missing something here.
Look forward to seeing this get in!
| ) | ||
|
|
||
|
|
||
| def _get_wires(val): |
There was a problem hiding this comment.
We could alternatively use the Wires class:
- check if val is an Operation, if so call (op.wires).to_set()
- else call w = qml.wires.Wires(val) and then return w.to_set()
There was a problem hiding this comment.
Yeah, I did think of leveraging Wires initially. Tried to simplify it a bit by using it.
| """A ``Conditional`` for evaluating if a given wire exist in a specified set of wires | ||
|
|
||
| Args: | ||
| wires (Union[list[int, str], Wires]): sequence of wires for building the wire set. |
There was a problem hiding this comment.
I think we should add validation for the wires argument for all of these classes? Since this is the condition, it doesn't get validated in _get_wires like everything else.
There was a problem hiding this comment.
The only invalid wires in PL would be the objects which are unhashable, which will be automatically caught by the set(wires) we do during instantiation.
| >>> cond_func = qml.noise.op_eq(qml.RX(1.0, "dino")) | ||
| >>> cond_func(qml.RX(1.23, wires=["eve"])) | ||
| True | ||
| >>> cond_func(qml.RY(1.23, wires=["dino"])) | ||
| False |
There was a problem hiding this comment.
This feels unintuitive to me, if I as a user provide an operation qml.RX(1.0, "dino"), I would expect that first if conditional to fail. I would want the conditional to not only match the operator, but also its arguments.
We could probably support this by leveraging qml.equal since it checks for programmatic equality.
pennylane/noise/conditionals.py
Outdated
| x = [x] if not isinstance(x, (list, tuple, set)) else x | ||
|
|
||
| try: | ||
| return all( | ||
| ( | ||
| _x in self._cops | ||
| if isclass(_x) | ||
| else ( | ||
| isinstance(_x, self._cops) | ||
| if not getattr(_x, "arithmetic_depth", 0) | ||
| else any( | ||
| _check_with_lc_op(op, _x) | ||
| for op in self._cond | ||
| if not isclass(op) and getattr(op, "arithmetic_depth", 0) | ||
| ) | ||
| ) | ||
| ) | ||
| for _x in x | ||
| ) |
There was a problem hiding this comment.
This logic isn't very "readable", I don't think the few lines we save from using tuple comprehension is worth it in this case. Consider re-writing with a loop and conditionals.
pennylane/noise/conditionals.py
Outdated
| if len(self._cops) == 1 | ||
| else len(x) == len(self._cond) | ||
| and all( | ||
| _check_with_lc_op(_op, _x) |
There was a problem hiding this comment.
Two comments:
-
I think the only arithmetic operators we can queue right now, are Prods, and Exp. Does it make sense to be checking using the linear combinations class if those operators don't have an LC representation, while the operators that do won't ever be queued as unitaries to begin with?
-
Does this assume a certain ordering? The comparison happening here is pairwise, I worry if the operator being queued is the same operation but with the summands in a slightly different order, this check would not catch that case.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5674 +/- ##
==========================================
- Coverage 99.68% 99.67% -0.01%
==========================================
Files 416 418 +2
Lines 39053 38538 -515
==========================================
- Hits 38929 38413 -516
- Misses 124 125 +1 ☔ View full report in Codecov by Sentry. |
Context: We aim to have a custom boolean function that could serve as a building block for creating noise models.
Description of the Change:
BooleanFnto work with arbitraryargsandkwargs.NoiseConditionalsas a class that helps buildBooleanFnrequired for creating noise models.wire/op_eq/into enhance the building experience for the aboveBooleanFn.Benefits:
Possible Drawbacks:
Related GitHub Issues: [sc-63080]