python: apply rewrite sequences loaded from JSON

microsoft / knossos-ksc

Compiler with automatic differentiation

Other

45 stars 10 forks source link

python: apply rewrite sequences loaded from JSON #916

Closed acl33 closed 3 years ago

acl33 commented 3 years ago

This address AB#20021 for the evaluation pipeline

Adds serialization to/from JSON(-able objects) to Path. (_FieldElement/_CallArgs raise exception if passed to json)
Also a test_path.py, as somehow there were no tests of Path before!)
Add a function rewrite_seq_to_exprs to rewrites.py, somewhat modelled on the RLO version.
Tweak PathElement's __str__ to be more consistent ("Let_rhs" or "Call_args[0]") I made the latter take serialized paths, but we could rely on the caller to deserialize first? This way the test does show what some serialized paths look like.

ryotatomioka commented 3 years ago

I think we need a high-level docstring that describes the design of PathElement or ExprWithPath.

It would be also nice to have an example of SerializedPath in the source code as comment / docstring. I imagine that it will be much larger than a single integer we use at the moment. What are the pros and cons of using a path?

acl33 commented 3 years ago

I think we need a high-level docstring that describes the design of PathElement or ExprWithPath.

It would be also nice to have an example of SerializedPath in the source code as comment / docstring. I imagine that it will be much larger than a single integer we use at the moment. What are the pros and cons of using a path?

Docstring + example, seems good (I think best as a test, to verify the doc stays correct - do we want to bring in doctest? Probably yes :), but now?).

"Pros and cons of using a path" I think @awf has argued the path will be more readable and debuggable and that the additional length does not matter.

acl33 commented 3 years ago

I think we need a high-level docstring that describes the design of PathElement or ExprWithPath.

Let's try to do that in #923 . I've rebased this to come after that PR, as this now implements the following suggestion:

It would be also nice to have an example of SerializedPath in the source code as comment / docstring.

acl33 commented 3 years ago

Hang on, that wasn't supposed to close - just trying to retarget this to go into master