Add parameter_names and parameter_symbols protocols. #3273
Conversation
cirq/protocols/resolve_parameters.py
Outdated
| if names is not NotImplemented: | ||
| return {sympy.Symbol(name) for name in names} | ||
|
|
||
| return set() |
There was a problem hiding this comment.
The code of these two functions is nearly identical. Do we really need both? At minimum I'd implement one of these in terms of the other, but maybe it's even better to leave the conversion up to the user. I imagine any client code can easily be adapted to work with either.
There was a problem hiding this comment.
I'd lean toward implementing parameter_symbols in terms of parameter_names, since strings are built-in. But that would incur extra overhead in some cases, e.g. if getting symbols from a sympy expression we would convert to strings and then back to symbols. Not a big deal, but something to consider.
We have a somewhat unfortunate dichotomy between strings and symbols throughout the parametrization code where strings and symbols are in 1:1 correspondence but one or the other is more useful in different cases. I'd rather not force users who want to work with symbols to always have to convert from strings ({sympy.Symbol(n) for n in cirq.parameter_names(val)}) and vice-versa, which is why I thought to add two functions.
| """Returns a collection of sympy Symbols of parameters that require | ||
| resolution. The collection is empty iff _is_parameterized_ is False. | ||
| """ | ||
|
|
There was a problem hiding this comment.
This looks to dissociate parameter names and from the symbos that need to be resolved? Would a better interface be _parameters_ which returns a map from string to symbol (and None if it doesn't require resolution)?
One reason to include parameter names even if they don't currently need resolution is this could be used to automatically populate serializated gate sets names.
Also I think if you did this you could end up using a decorator on classes which could autohandle creating these methods: Those that follow the pattern of having args in init equal to attributes and all are parameters could always to this.
There was a problem hiding this comment.
The two methods are just to get parameters in the format most convenient for the calling code. Names and symbols are in 1:1 correspondence and it's easy to convert between them, e.g. symbol.name <-> sympy.Symbol(name) so I'm not sure that returning a mapping here would help all that much. Maybe if there were bi-directional maps in python since different callers might want to convert in different directions.
There was a problem hiding this comment.
Ah, wait, I think I misunderstood your comment. You're imagining returning something like {'angle': sympy.Symbol('foo'), 'exponent': None} to indicate that we have "parameters" angle and exponent, and a symbol foo that needs to be resolved? I see some problems with that sort of approach.
- For purposes of the SupportsParametrization protocol,
foois what we mean by "parameter" whileangleandexponentare something else, sort of like slots in which parameters may or may not be put. - "Slots" like
angleorexponentcould actually contain expressions involving many symbols that need to be resolved, so at least we'd need to return aMapping[str, Set[sympy.Symbol]]. - If analyzing a composite object like a circuit, the "slots" are defined on sub-objects like gates within the circuit, so we refer to such slots without also specifying which gate at which position in the circuit; there are no slots on the circuit itself. The idea of the protocols I'm proposing is to strip away all this structure and just get a "flattened" set of parameters that need to be resolved anywhere within the composite object.
|
I changed the |
There was a problem hiding this comment.
This looks good. I would have probably separated out the changes related to resolve_parameters (both in the protocol and in implementing classes) so that they don't end up in the same commit - but it's not the end of the world.
The number check is one bit I'd like to understand better before approving, see comments.
| @@ -55,6 +66,8 @@ def is_parameterized(val: Any) -> bool: | |||
| """ | |||
| if isinstance(val, sympy.Basic): | |||
| return True | |||
| if isinstance(val, numbers.Number): | |||
There was a problem hiding this comment.
This is an interesting case which probably should be handled, but I'm not sure if this is the right place. What cases were you trying to capture? I don't see it covered in the tests.
I think this should come first.
>>> isinstance(sp.sympify("1"), numbers.Number)
True
>>> isinstance(sp.sympify("1"), sp.Basic)
True
>>> isinstance(sp.sympify("a + 1"), sp.Basic)
True
>>> isinstance(sp.sympify("a + 1"), numbers.Number)
False
There was a problem hiding this comment.
Moved this to be the first thing checked before sympy.Basic. I don't recall exactly which failures this fixed, but some of the existing classes that implement the resolve parameters protocol did so incorrectly. This gets covered by some of the tests I've added that check for consistent implementation of the resolve parameters protocol, but I will add some more specific unit tests.
There was a problem hiding this comment.
That could be. I think I am the one who added this to parameter resolution so that it avoids sympy.subs which is slow as molasses. The goal was that not too much should fall through to sympy's formula resolution for performance reasons. It's totally possible I goofed and didn't implement sympy's typing correctly or in the right order.
There was a problem hiding this comment.
Hm, so putting the Number check first results in some failures because we have code that assumes that if code is not parameterized then it's possible to do floating point operations on it. For example, in TwoQubitDiagonalGate if the gate is not parameterized we try to compute the unitary (https://github.com/quantumlib/Cirq/blob/master/cirq/ops/two_qubit_diagonal_gate.py#L63), but this fails if the gate contains sympy expressions even if there are no free parameters. I guess to model this properly we'd need separate checks for "is symbolic" versus "is parameterized". For now, I'm going to leave the number checks after the sympy checks.
There was a problem hiding this comment.
OK, added #3310 for further work.
| """ | ||
| if isinstance(val, sympy.Basic): | ||
| return {symbol.name for symbol in val.free_symbols} | ||
| if isinstance(val, numbers.Number): |
There was a problem hiding this comment.
I would move this up to be first as well.
| except Exception: | ||
| pass | ||
| else: | ||
| print('names:', names) |
There was a problem hiding this comment.
I assume these print statements were only for debugging, let's remove them?
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
For various internal applications, we'd like to be able to determine not just whether an object is parametrized, but rather what are the actual parameters it has that must be resolved. This adds two new protocol methods in the SupportsParametrization protocol to get the parameters as strings (
parameter_names) orsympy.Symbols(parameter_symbols). Parameterized classes can implement one or both of the magic methods_parameter_names_and_parameter_symbols_; only one is needed since we can easily convert names to symbols and vice-versa.For now I've just implemented the methods and I'd like to get feedback on the API before I fix up tests, docs, etc.