Allow custom definition for _sympy_pass_through #3921

zchen088 · 2021-03-17T02:36:45Z

maffoo

This seems reasonable to me. Once we settle on the design, we should update the value_of docs (and probably the resolve_parameters protocol docs) to describe this mechanism and how it can be used on custom types that one might want to use when resolving parameters.

maffoo · 2021-03-17T04:04:22Z

cirq/study/resolver.py

@@ -244,4 +244,9 @@ def _sympy_pass_through(val: Any) -> Optional[Any]:
        return val.p / val.q
    if val == sympy.pi:
        return np.pi
+
+    getter = getattr(val, '_sympy_pass_through_', None)


I think it'd be helpful to change the name, because the point of defining a special method is to broaden this beyond the hard-coded sympy handling. In addition, the code above allows transforming values, not just passing them through (e.g. sympy integers become plain old integers). So maybe we could call the special method something like _resolver_value_ and if it exists we call it to get a value to pass through, which could be different from val itself. The method could return NotImplemented to indicate that the value should not be passed through. We could also change this function itself to use NotImplemented as the sentinel value instead of None if we want to allow things to resolve to None (the function is only called in value_of above, so that change wouldn't affect any other code).

We could also change this function itself to use NotImplemented as the sentinel value instead of None if we want to allow things to resolve to None (the function is only called in value_of above, so that change wouldn't affect any other code).

I tried this but it turns out that we're relying on None as a sentinel in some subtle way that I couldn't quite unravel, probably because of this line: https://github.com/quantumlib/Cirq/blob/master/cirq/study/resolver.py#L175

That None was me :p A valid workaround would be to replace None with a file-local class used only for flagging potential loops in recursive parameter evaluation.

Hmm...I guess replacing wouldn't resolve this entirely, since None is checked for earlier in value_of.

@95-martin-orion thanks for the hint, got all the tests to pass now, check if this is what you had in mind?

I'd put a leading underscore on RecursionFlag just to make it abundantly clear that it's a non-public object, but otherwise this looks right.

+1 to leading underscore. I'd also suggest adding a comment that this is used to mark values that are being resolved recursively to detect loops.

maffoo · 2021-03-17T15:21:59Z

cirq/study/resolver_test.py


    class Bar:
-        def _sympy_pass_through_(self):
-            return False
+        pass


maybe add a case here that has _resolver_value_ method that returns NotImplemented?

cirq/study/resolver_test.py

95-martin-orion · 2021-03-17T15:37:57Z

This seems reasonable to me. Once we settle on the design, we should update the value_of docs (and probably the resolve_parameters protocol docs) to describe this mechanism {...}

Our requirement for "magic methods" (i.e. _functions_like_this_) is that they have an associated protocol. For _resolve_parameters_, this protocol is SupportsParameterization:

Cirq/cirq/protocols/resolve_parameters.py

Line 28 in 8cf7825

class SupportsParameterization(Protocol):

To follow the same rule for _resolver_value_, we should add a protocol (in resolve_parameters.py is fine) for that method.

maffoo · 2021-03-17T15:44:30Z

@95-martin-orion, the problem with adding this in SupportsParametrization is that this new method _resolver_value_ is for values that we want the resolver to deal with, not parametrized objects themselves. We could add a new ResolvableValue protocol for this, but while we have top-level methods to invoke most protocols (like cirq.resolve_parameters for SupportsParametrization) I don't think it makes sense to add a top-level method in this case. It's really an internal detail of how the existing parameter resolution protocol works.

95-martin-orion · 2021-03-17T15:49:24Z

@95-martin-orion, the problem with adding this in SupportsParametrization is that this new method _resolver_value_ is for values that we want the resolver to deal with, not parametrized objects themselves. We could add a new ResolvableValue protocol for this, but while we have top-level methods to invoke most protocols (like cirq.resolve_parameters for SupportsParametrization) I don't think it makes sense to add a top-level method in this case. It's really an internal detail of how the existing parameter resolution protocol works.

Protocols do not require top-level methods - see SerializableByKey (although, due to imnplementation details, this protocol no longer has a "magic method" associated with it):

Cirq/cirq/protocols/json_serialization.py

Line 330 in 06e4892

class SerializableByKey(SupportsJSON):

maffoo · 2021-03-17T16:04:17Z

cirq/study/resolver.py

@@ -237,6 +237,16 @@ def _from_json_dict_(cls, param_dict, **kwargs):
        return cls(dict(param_dict))


+class ResolvableValue(Protocol):
+    @doc_private
+    def _resolver_value_(self) -> Any:


Maybe we should call it _resolved_value_ to match what's in the docstring below (I think that explanation is pretty clear). Also, I'd suggest moving this protocol into resolve_parameters.py alongside the other protocols that are related to parameter resolution, so we can document the whole process there.

maffoo · 2021-03-17T16:04:55Z

cirq/study/resolver.py

+        further parsing like we do with Sympy symbols.
+        """
+
+
 def _sympy_pass_through(val: Any) -> Any:


Rename this _resolve_value?

cirq/study/resolver.py

95-martin-orion

One remaining nit, then this looks good to go.

95-martin-orion · 2021-03-17T17:54:44Z

cirq/study/resolver_test.py

@@ -230,6 +230,32 @@ def test_resolve_unknown_type():
    assert r.value_of(cirq.X) == cirq.X


+def test_custom_sympy_pass_through():


test_custom_sympy_pass_through -> test_custom_resolved_value

maffoo

LGTM

Allow custom definition for _sympy_pass_through

2adea3d

zchen088 requested review from maffoo and balopat March 17, 2021 02:36

zchen088 requested review from cduck, vtomole and a team as code owners March 17, 2021 02:36

google-cla bot added the cla: yes Makes googlebot stop complaining. label Mar 17, 2021

maffoo reviewed Mar 17, 2021

View reviewed changes

Review

22b73dd

maffoo reviewed Mar 17, 2021

View reviewed changes

Review

55ba1b7

zchen088 requested review from 95-martin-orion and maffoo March 17, 2021 15:23

95-martin-orion reviewed Mar 17, 2021

View reviewed changes

cirq/study/resolver_test.py Outdated Show resolved Hide resolved

Uncomment test

de51eed

Add Protocol?

bb4c004

Fix import

9400b9a

maffoo reviewed Mar 17, 2021

View reviewed changes

zchen088 added 2 commits March 17, 2021 09:09

Review

52dca03

Merge branch 'master' into 3916-sympy-pass-through

78e27ad

zchen088 requested a review from maffoo March 17, 2021 16:16

zchen088 added 3 commits March 17, 2021 09:18

Review

ecaff2d

Lint

5b11179

Merge branch 'master' into 3916-sympy-pass-through

9bd5862

95-martin-orion reviewed Mar 17, 2021

View reviewed changes

cirq/study/resolver.py Outdated Show resolved Hide resolved

Review

ddec8de

Test the case I missed

9632a0e

95-martin-orion approved these changes Mar 17, 2021

View reviewed changes

Review

ea9b3df

maffoo approved these changes Mar 17, 2021

View reviewed changes

zchen088 added 2 commits March 17, 2021 11:24

Dummy commit

7fee2c1

Dummy commit

b06d550

zchen088 merged commit 38cb2f2 into quantumlib:master Mar 17, 2021

zchen088 deleted the 3916-sympy-pass-through branch March 17, 2021 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom definition for _sympy_pass_through #3921

Allow custom definition for _sympy_pass_through #3921

zchen088 commented Mar 17, 2021

maffoo left a comment

maffoo Mar 17, 2021

zchen088 Mar 17, 2021

95-martin-orion Mar 17, 2021

95-martin-orion Mar 17, 2021

zchen088 Mar 17, 2021

95-martin-orion Mar 17, 2021

maffoo Mar 17, 2021

maffoo Mar 17, 2021

95-martin-orion commented Mar 17, 2021

maffoo commented Mar 17, 2021

95-martin-orion commented Mar 17, 2021

maffoo Mar 17, 2021

maffoo Mar 17, 2021

95-martin-orion left a comment

95-martin-orion Mar 17, 2021

maffoo left a comment

		@@ -230,6 +230,32 @@ def test_resolve_unknown_type():
		assert r.value_of(cirq.X) == cirq.X


		def test_custom_sympy_pass_through():

Allow custom definition for _sympy_pass_through #3921

Allow custom definition for _sympy_pass_through #3921

Conversation

zchen088 commented Mar 17, 2021

maffoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

95-martin-orion commented Mar 17, 2021

maffoo commented Mar 17, 2021

95-martin-orion commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

95-martin-orion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maffoo left a comment

Choose a reason for hiding this comment