[WIP] broadcast arguments mixin

[alexanderivrii]

Summary

Creating a mixin for the "broadcast arguments" functionality

Details and comments

This is the first (small) step towards adding a Clifford (and other objects) to a QuantumCircuit natively, i.e. as an Operation, without having to convert this clifford to Instruction first (by synthesizing a quantum circuit for it).

The "arguments broadcasting" functionality is a handy shortcut to add multiple gates at once, for instance
qc.cx(0, [1, 2, 3]) is a shortcut for

qc.cx(0, 1)
qc.cx(0, 2)
qc.cx(0, 3)

Currently broadcast_arguments is explicitly implemented in Instruction, Barrier, Delay, Gate, Measure, Reset and Initializer. Some of these functions are very different, and some are exactly the same (e.g., for barrier and delay).

I would like to get your feedback if this is the right approach to create the broadcaster mixin. I have moved all broadcast_arguments functionality to a separate file, and each relevant class (e.g., Gate) specifies the correct ArgumentsBroadcaster mixin in its definition.

(Please ignore some printing statements and the additional test file for now).

One additional question is about naming things. Would it make sense to rename ArgumentsBroadcaster to something more intuitive, like ArgsExpander? Would it make sense to combine classes with the exact same functionality, like ArgumentsBroadcasterBarrier and ArgumentsBroadcasterDelay (and call it something suited for both)? Does it make sense to call ArgumentsBroadcasterGeneric for the functionality previously present in Instruction?

[jakelishman]

Adding more mixins is going to exacerbate the performance regressions that we saw with the original PR, and I'm not certain what the benefit is. I don't see why this would need to be a mixin - it can be a regular method on the existing Operation mixin in the same way it is now on Instruction. If you want to re-use the functions, you can define them elsewhere, and each class can just bind one in as it chooses.

This said, I've actually been thinking about making a much larger restructuring of broadcast_arguments. Currently, there's a very awkward split between "resolution of qubit specifiers" (like int to Qubit, etc) and "broadcasting of arguments" within QuantumCircuit. There's no defined expectations between the two, and different implementations of Instruction.broadcast_argument handle errors, style and allowed inputs differently. The biggest problem with it for me, though, is that it requires an instance of the instruction to broadcast, and that's where the real issues come in: QuantumCircuit needs to know how many qubits and clbits each instance should take in order to construct the instance, but it can't know what qubits and clbits it'll be applying the Operation to until it has the instance.

At the moment, there's a bunch of undefined assumptions (not all of which are compatible, I believe), and I can only assume it's buggy anyway: for example, QuantumCircuit.append makes a deepcopy of an Instruction that comes in with Parameters (but not the more general ParameterExpression?), but then re-uses the same instance for each element of the broadcast, so whatever problem it was trying to solve won't survive that case.

---

In somewhat summary: the only change I think you need for your goals is simply to add a broadcast_arguments function directly to Operation. I'm still thinking through where the split in broadcast_arguments should really be on a larger scale, and I'm not keen on separating it into a mixin interface. Right now I'm not entirely convinced it shouldn't just be entirely the responsibility of QuantumCircuit, and the different classes may select the desired behaviour using a class-level attribute from a fixed enum.

[coveralls]

Pull Request Test Coverage Report for Build 2028275236

• 89 of 90 (98.89%) changed or added relevant lines in 8 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.01%) to 83.543%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/broadcast.py	72	73	98.63%

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/delay.py	1	91.53%

Totals
Change from base Build 2027366818:	0.01%
Covered Lines:	52639
Relevant Lines:	63008

---

💛 - Coveralls

[kdk]

Adding more mixins is going to exacerbate the performance regressions that we saw with the original PR, and I'm not certain what the benefit is. I don't see why this would need to be a mixin - it can be a regular method on the existing Operation mixin in the same way it is now on Instruction. If you want to re-use the functions, you can define them elsewhere, and each class can just bind one in as it chooses.

This was intended as a way to facilitate code reuse across different families of Operations, especially around behavior that was considered optional, like broadcast_arguments. Functions are also possible (though with their own tradeoffs and questions), but I am somewhat hesitant to drive design decisions based on the regressions from #7528 without a better understanding their scope or capacity for growth.

In wall time, each of the regressions reported were <5ms with the exception of passes.MultipleBasisPassBenchmarks.time_basis_translator which was ~40-60ms (but these will already be mitigated by #7211 ). If there is reason to think that these numbers will grow with either the number of operations, number of mixins, or anything else, I'd agree we should reexamine this approach, but failing that, the mixins provide a useful way of consolidating and unifying some of the existing disparate and inconsistent behavior we have for Instruction types like broadcast_arguments, even if only as a temporary stepping stone towards something more organized.

Also, agree this does seem like a very good time to at least move broadcast_arguments to be an @classmethod, or think about standardizing to a fixed set of behaviors owned by QuantumCircuit.

[jakelishman]

Mostly my issue is that mixins shouldn't be the default way to re-use code - that makes sense in a compiled language where there's no run-time penalty for extending the lookup paths, but in Python everything is dynamic and there are more options for importing code into classes. There's more unnecessary complications with mixins (we need to make sure every mixin defines __slots__ = (), for example) compared to simply binding a function, and they don't play as nicely with inheritance.

If we want to share functions between classes, it can be as simple as having a module broadcast.py:

@staticmethod
def numpy_like(qargs, cargs):
    qargs = [(qarg,) if isinstance(qarg, Qubit) else tuple(qarg) for qarg in qargs]
    cargs = [(carg,) if isinstance(carg, Clbit) else tuple(carg) for carg in cargs]
    all_args = qargs + cargs
    split = len(qargs)
    n_returns = max(all_args, key=len)
    broadcast_args = []
    for arg in all_args:
        if len(arg) == n_returns:
            broadcast_args.append(arg)
        elif len(arg) == 1:
            broadcast_args.append(arg * n_returns)
        else:
            raise CircuitError("incorrect lengths in broadcast")
    for out_args in zip(*broadcast_args):
        yield out_args[:split], out_args[split:]

@staticmethod
def none(qargs, cargs):
    qargs, cargs = tuple(qargs), tuple(cargs)
    if not (all(isinstance(qarg, Qubit) for qarg in qargs) and all(isinstance(carg, Clbit) for carg in cargs)):
        raise CircuitError("this instruction does not support broadcasting")
    yield qargs, cargs

...

and then in the individual classes it just goes like

class Operation(ABC):
    broadcast_arguments = broadcast.none
    ...

class Instruction(Operation):
    broadcast_arguments = broadcast.none

class Gate(Instruction):
    broadcast_arguments = broadcast.numpy_like

This has the same code cost to the author as a mixin, but it uses the standard lookup rules. Mixins are a good pattern when there's a few parts that need to interact with each other, but if it's just about adding a single function, it feels like binding in a single value has all the advantages and none of the downsides.

For a major downside, consider this:

In [1]: class InstructionBroadcastMixin:
   ...:     @staticmethod
   ...:     def broadcast():
   ...:         return "instruction-like"
   ...: class GateBroadcastMixin:
   ...:     @staticmethod
   ...:     def broadcast():
   ...:         return "gate-like"
   ...:
   ...: class Instruction(InstructionBroadcastMixin):
   ...:     pass
   ...: class Gate(Instruction, GateBroadcastMixin):
   ...:     pass
   ...:
   ...: (Instruction.broadcast(), Gate.broadcast())
Out[1]: ('instruction-like', 'instruction-like')

Mixins don't play nicely with overloading, because the primary lookup for Gate should be its parent (Instruction), but then if you have the mixins afterwards, they don't override anything because the lookup never reaches them. The programmer has to remember to define a class's parents out of logical order in this form.

[alexanderivrii]

@jakelishman , @kdk, thanks for your comments (and sorry for a long reply).

I have implemented the approach suggested by @jakelishman and have been running performance regression using asv on my (Windows10) laptop. specifically on passes.MultipleBasisPassBenchmarks.time_basis_translator and passes.MultiQBlockPassBenchmarks.time_collect_multiq_block.

If anyone is interested, the results are attached below. I have run the 3 versions (main vs. this PR vs. approach based on functions) three times each on both passes above, and I don't really see a noticeable difference between these; what makes the comparison a bit problematic is that I get slightly different results even when running the same thing multiple times (either because it runs on the laptop, or because the runs are inherently non-deterministic, though I thought that this was recently fixed using transiler_seed).

broadcast_asv.txt

Are there any more tests I can do before we can decide which approach to go with?

As @kdk said, we want to move auxiliary functionality completely out of Instruction/Operation. Personally, I don't have any preference whether they should be implemented as mixins or functions. It would be good to have them all sitting in a single file, so that we can directly compare and reuse different implementation for other objects. I completely agree with

different implementations of Instruction.broadcast_argument handle errors, style and allowed inputs differently

Regarding Python caveats, I believe mixins are expected to appear first, so there should be no problem to do things in the right order.

Actually, @jakelishman, I may have misinterpreted your suggestion:

class Gate(Instruction):
    broadcast_arguments = broadcast.numpy_like

I have added the line in __init__, i.e. self.broadcast_arguments = broadcast.numpy_like (where one has to be even more careful to put this line after the call to super().__init__), but now I see that your suggestion was to put it right in the class definition.

[jakelishman]

You're right, mixins should go first. My point is that it's just one example of increased cognitive load for something most people don't think about. It requires extra load to think "these classes are mixins that need to go first, then this class is the actual hierarchical parent, then these classes are additional interfaces that I define and need to go last", and then it makes it quite hard to look at a class and see what its logical parent really is. It's possible, but I really can't see any advantages in this case over direct method binding, and "making things harder to think about" is an important disadvantage of itself.

I am slightly surprised that there seems to be no speed impact, though I do wonder if that would hold for much larger inheritance trees, especially including resolution of methods that are defined low in the tree. It's one more example in the quiver of the "no premature optimisation" crowd, though.

Yeah, exactly, the example I gave was pretty verbatim of what I meant - methods should be bound in the class scope, not the instance scope, no matter where they come from. The reason is that

class A:
    def my_method(self):
        pass

is (very nearly) identical to doing

def _my_method(self):
    pass

class A:
    my_method = _my_method

That's why

class Gate(Instruction, Operation):
    broadcast_arguments = broadcasters.numpy_like

    def __init__(self, ...): ...

is the same as if it had been written as

class Gate(Instruction, Operation):
    def broadcast_arguments(...):
        # ... the same code as `broadcasters.numpy_like` ...
    
    def __init__(self, ...): ....

Binding methods within __init__ isn't a proper method definition, e.g. you wouldn't be able to access it from the class. (There's also some very technical differences that matter about how class-defined values are retrieved from an object as opposed to instance-defined values, but I don't want to derail things.)

I appreciate that the confusion on that point suggests it may also not be familiar to everyone, but I'd argue that regular method binding is still simpler long term and unlikely for people making new classes to get it wrong, since they'll just copy the existing styles and it doesn't need to interact with other elements in the MRO.

[alexanderivrii]

@jakelishman, thanks for the explanations! I agree that with your approach we don't need to worry about inadvertent performance slowdown. Though I do believe that it's easier to look at class definition, e.g., at class Gate(ArgumentsBroadcasterGate, Instruction, Operation) to immediately understand where the functionality comes from, without having to search for methods defined at class scope.

[eliarbel]

I think that mixins provide a cleaner way to extend a class interface, using the well known concept of inheritance. Furthermore, beyond just the discussion of style preference, I find 2 annoying disadvantages with class method binding approach:

1. scanning the code within the class definition itself to identify the class attributes and class bound methods is not convenient, unless the class is very simple and everything fits into the screen without too much clutter around the binding code. With a well documented code, this is probably not the case
2. IDEs (at least PyCharm) does not handle nicely bound methods, the way it handles inherited (e.g. mixin) method, from code completion perspective.

In addition, using quite unique names in the arg broadcasting mixins, chances for hitting name collisions during MRO are probably practically 0

[jakelishman]

For point 1: I'm not sure I understand how looking for these methods is any harder than looking for a normal method? Instruction already defines some class-level properties like this, and they're just defined at the top of the class - very easy to see. This could just be my personal style, though, as I rarely scan for methods - I just search the file or use my editor's jump-to-definition function.

For point 2: if PyCharm can't complete a method bound in this way, it might be a bug - Jedi-based completers (IPython, Jupyter, my editor plugin, etc) don't seem to have a problem with it, though I might be missing some of what you mean. This binding method is also how some QuantumCircuit methods are already defined (unfortunately), such as the methods that Aer monkey-patches on. (Not that I particularly like that design.) It's a well documented, regular feature of Python.

For name collisions: I'm not 100% sure what potential issues you had in mind, but I completely agree there's no chance of any unwanted collisions here.

---

I do fully accept that mixins are useful in many situations, but I think they're for additive code only, not for situations where we need to be able to override them later. I'm not sure that the design of Python fits with using mixins to define simple functions that may need to be overridden at several different levels, though it may well in other more statically dispatched OOP languages.

For a real example in the current codebase: Gate is a subclass of Instruction, and both of these classes define their own broadcasting rules. In this new scheme, that looks something like

class InstructionBroadcast:
    def broadcast_arguments(): ...
class GateBroadcast:
    def broadcast_arguments(): ...

class Instruction(InstructionBroadcast):
    pass
class Gate(GateBroadcast, Instruction):
    pass

So far so good - Gate correctly overrides Instruction with the new broadcast method.

However, now the user wants (or we want) to define a gate UserGate that uses the broadcast method from Instruction instead. An example of this in Terra is Initialize¹. In the spirit of code re-use we want them to use the mixin, but this is impossible:

In [2]: class UserGate(InstructionBroadcast, Gate):
   ...:     pass
   ...:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [2], in <cell line: 1>()
----> 1 class UserGate(InstructionBroadcast, Gate):
      2     pass

TypeError: Cannot create a consistent method resolution
order (MRO) for bases InstructionBroadcast, Gate

The grandchild class can't use the mixin InstructionBroadcast, because the MRO would need to be

• UserGate
• InstructionBroadcast (needs to be before Gate because of UserGate)
• Gate
• GateBroadcast
• Instruction
• InstructionBroadcast (needs to be after Instruction because of Instruction)

but this is invalid - you can't have the same class in the MRO twice.

As far as I'm aware, there's no way around this with a mixin structure. The only alternative (I can think of - I could be wrong) is to manually define the broadcast_arguments method on UserGate, but with the behaviour now wrapped up in mixin classes, that would look like

class UserGate(Gate):
    def broadcast_arguments(self, qargs, cargs):
        return InstructionBroadcast.broadcast_arguments(self, qargs, cargs)

which is quite similar to the method I suggested of directly binding the functions into the class, except more verbose, it doesn't inherit the docstring, and the code reader has to check for two possible definition patterns rather than just one. The last point is particularly confusing when some classes will use "lack of mixin" to mean "inherit", and others will use "lack of mixin" to mean "I'm overriding this with a grandparent method".

This is a situation where classes may very well want to use the same code that a grandparent does - broadcast_arguments is meant to be defined on any class that can be added to QuantumCircuit, and its behaviour is completely independent of what other behaviour the gate is supposed to inherit; Gate.broadcast_arguments is meant to be the default for gates, not a guarantee that all subclasses will behave like a specialised version of this. It's completely valid that some Gate subclass would want to use the broadcast_arguments that happens to be the default for its grandparent Instruction, but the mixin structure forbids this. I think this is the key point that makes mixins (to me) unsuitable for this task.

Footnotes

1. In principle, maybe not quite in reality: I think the definition of Initialize.broadcast_arguments is meant to mean "no broadcasting" just like Instruction's, but the code itself is a bit funky and different. ↩

[jakelishman]

In the language of OOP design patterns, I'd call my suggested method a simpler version of composition (Python lets us compose without creating inner objects, unlike traditional OOP), as opposed to the mixin structure being an example of inheritance. If it matters, my understanding is that modern OOP design has been moving in the direction of re-use by composition over inheritance for a while, but I'm getting a little out of my wheelhouse here - I come from a background of procedural and then functional programming, not object-oriented.

For "the well known concept of inheritance": in my mind at least, my suggested method does this as well - it really is identical to the current way that broadcast_arguments is defined on classes and inherited or overridden by subclasses, but with composition by function re-use. If anything, I'd perhaps even call mine more in the "spirit" of pure inheritance than mixins (since it's singly inherited and doesn't have restrictions on overriding), though I appreciate that Python's flexibility in method definitions can make it look otherwise to those used to more rigid languages like C++. In Python, they really do bind in exactly the same way (except for __qualname__):

In [1]: def _f(self):
   ...:     pass
   ...:
   ...: class A:
   ...:     f = _f
   ...:
   ...:     def g(self):
   ...:         pass
   ...:

In [2]: A.f
Out[2]: <function __main__._f(self)>

In [3]: A.g
Out[3]: <function __main__.A.g(self)>

In [4]: A().f
Out[4]: <bound method _f of <__main__.A object at 0x000002131F64F8B0>>

In [5]: A().g
Out[5]: <bound method A.g of <__main__.A object at 0x000002131F74A3D0>>

[eliarbel]

Thanks Jake. Your example with UserGate and the issue it creates with MRO helps us moving forward: the problem with the current mixin suggestion is that it's not a single mixin for the entire hierarchy tree of Instruction Gate etc but rather a set of mixin classes. This is probably not a good use-case for the mixin pattern.
I still find the bound methods approach though somewhat less elegant than relying on inheritance, but I agree it's subjective. I can expand BTW more on the issues I find with it with, e.g. with code editors, but probably that's too much detail for this thread. We can discuss over Slack if you want.
Anyway we'll roll-back from the mixin family approach and use an alternative that will not block the broadcasting method overrides use-case as you've given above.

[jakelishman]

Thanks Eli. Sorry I wasn't able to work out the issue a bit sooner in the conversation to save time - I hadn't originally spotted it (and I wasn't 100% against mixins for this usage before that point, just trying to find simpler ways).

It'd be good to talk in person just in general, but unfortunately it'll have to wait a few days more - I'm away from work finishing my PhD thesis, and I'm just commenting on some GitHub issues to take a break from writing every so often. I'll be at the dev meetup in April, though.

[alexanderivrii]

Eli and Jake, thanks! As Jake's example with UserGate decisively shows, mixins are not the right strategy to implement broadcasting (in particular, we would not be able to efficiently reuse code).

In a private message, Eli has also suggested an alternative approach. We would (1) create a class ArgBroadcaster containing all possible broadcasting strategies:

class ArgBroadcaster:
    def broadcastInstructionArgs(self, qubits, clbits):
        pass

    def broadcastGateArgs(self, qubits, clbits):
        pass

(2) inherit Instruction from ArgBroadcaster:

class Instruction(ArgBroadcaster):

and (3) change all the existing broadcast_arguments methods to call a relevant method from this class:

class Instruction(ArgBroadcaster):
    def broadcast_arguments(self, qubits, clbits):
        self.broadcastInstructionArgs(qubits, clbits)

class Gate(Instruction):
    def broadcast_arguments(self, qubits, clbits):
        self.broadcastGateArgs(qubits, clbits)

class UserGate(Gate):
    def broadcast_arguments(self, qubits, clbits):
        self.broadcastInstructionArgs(qubits, clbits)

This works, however the extra function call does seems to slightly slow down things.

In this new reimplementation, I have almost completely adopted Jake's solution, yet did put all the relevant functions in a single class, i.e. the code is essentially this:

class Broadcaster:
    @staticmethod
    def instruction(instr, qargs, cargs):
        pass 

    @staticmethod
    def gate(instr, qargs, cargs):
        pass

class Gate:
    broadcast_arguments = Broadcaster.gate

class Instruction:
    broadcast_arguments = Broadcaster.instruction

Eli, Jake, what do you think?

I have a few additional questions:

• I don't really understand why this is called broadcasting (and not something like expanding arguments). Can we rename Broadcaster to ExpandArgs?
• The class Instruction also has a global class attribute _directive, which any "high-level-object" must also support. Previously, we thought to make a mixin out of this, but now it seems that class methods and class attributes may be the best way to do things (code reuse-wise and performance-wise). Jake, do you agree? I may not have fully understood you yesterday, did you say that for other usecases mixins still make sense? Wouldn't we always run into very similar problems as for broadcasting?
• With the current implementation, when somebody implements a new "high-level-object" (something like Clifford), and does not implement broadcast_arguments and _direction, we only get errors much later (e.g., for broadcasting -- when we try to add this object to a quantum circuit). On the other hand, mixins, abtractmethods, etc. allow to pinpoint such problems much earlier, yet seem to degrade performance and code reuse. Is there a simple solution to this? (And if not, how would anyone even know that his/her high-level object requires various class methods and attributes?)
• I did not try to reuse / cleanup broadcasting code in any way yet. Should we do this as part of this PR?

P.S. the reason why we are discussing broadcasting so much is that (I am hoping) whichever solution we adopt for this extra functionality will be also the solution to all other extra functionalities,

[jakelishman]

I don't have any technical issues with this new style you're suggesting. I personally don't see the reason for the containing classes - it feels quite like using a workaround only needed in pure-OO languages, because in Python static methods in classes are not functionally any different to regular methods in modules (if you call the module broadcast.py, you can still just from . import broadcast and do broadcast_arguments = broadcast.gate). But that's just a matter of style, and isn't important.

On _directive: yeah, I agree.

I think in your third bullet point you might have misunderstood me a little - I think we're much more in agreement already. I totally agree that Operation should gain abstract methods for anything that needs to be defined by a higher-order object. So I'd expect that Operation would do

class Operation(abc.ABC):
    @abc.abstractmethod
    def broadcast_arguments(self, qargs, cargs):
        raise NotImplementedError

    @abc.abstractmethod
    @property
    def _directive(self):
        raise NotImplementedError

    @property
    def name(self):
        raise NotImplementedError

    # ... and so on ...

This shouldn't have any impact on performance beyond what Operation already does (and even then we don't fully understand why there's a drop), if subclasses override _directive and broadcast_arguments with class-level attributes - the way Python method resolution works, there really should be no performance impact at all from that.

I agree with you that Operation should have those methods, and completely define the protocol a higher-order object must fulfill. I'd only meant to challenge mixins for code re-use (in these specific instances), rather than disagreeing with the Operation protocol. So I would vote to have Operation like you've already done, with the additions I just wrote above, and then do the class-level attributes on child classes like we've been talking about:

class Instruction(Operation):
    _directive = False
    broadcast_arguments = [bB]roadcast.no_broadcasting

That satisfies the definition of the protocol and all the abstract methods, and shouldn't cause any errors or slowdown on instantiation.

---

I think with broadcasting we need to be careful not to break API compatibility, so if we rename the method, perhaps Operation should start off life with its base method just calling the existing one.

I don't know the original reason it's called broadcasting, but I imagine it was by analogy to Numpy's terminology: add(arr1, arr2) broadcasts the operation add over missing dimensions in the two arrays, so if arr1 has a shape of (4, 5) and arr2 has a shape of (4,), arr2 will first be duplicated to make it up to (4, 5), and then the operation goes element-wise. The OpenQASM 2 and 3 specifications use "broadcast" to mean the same thing as well, so it might be a bit surprising to change the name.

[eliarbel]

Thanks Sasha. If we're going with method binding approach, I also don't see extra benefit with encapsulating static methods under a single class, rather than just putting those method in a single file. It may create a false perception that there is some intent to have those method share something at the class level (ArgBroadcaster).

Another point regarding Operation: I think we should carefully consider which Instruction functionality should move Operation vs somewhere else. Moving too much will just lead us to eventually renaming Instruction to Operation. Specifically regarding this PR, I consider argument broadcasting as a syntactic sugar feature really and not a requirement of Operation, from circuit element perspective. Given that I don't think we should have broadcast_arguments as an @abstratmethod in Operation.

Finally, a Mixin could be nice for argument broadcasting as a way to attach this extra functionality to circuit elements, but we agreed recently this has limited flexibility when it comes to choosing which broadcasting alg to use for which class. The approach I suggested as a replacement (single class with multiple instance methods, one for each broadcaster), is inspired by the Visitor design pattern. But this apparently also has performance penalty. My main point here though is that I'm curious to see how much run-time penalty this approach has vs using bound methods, I mean is it negligible or dramatic? I would argue that if we really want to have our code design process guided by performance considerations than we should at least measure performance in a somewhat more realistic setting, i.e with full-fledged circuits containing a lot of gates, rather than relying on microbenchmarks that may give as a skewed picture.

[eliarbel]

I think BTW that we would benefit from planning a bit how Instruction deprecation would look like from code refactoring point of view, before making local decisions without seeing the big picture.

[alexanderivrii]

Based on this discussion, I have added the proposed changes to the more general PR #7966.

Closing this PR.