search

microsoft / knossos-ksc

Compiler with automatic differentiation
Other
45 stars 10 forks source link

Make str(ASTNode) return an s-exp #917

Closed acl33 closed 3 years ago

acl33 commented 3 years ago

...so it's machine-parseable. (This is needed for end-to-end RLO run: str() is used to format Exprs into logs, from where they must be parsed. Hence addresses AB#19126.)

I'm a bit surprised that (almost) no tests break, but I'm not sure that adding a test that str == pformat here would really help.

I've repurposed and adapted the old __str__ code into __repr__. As per the comment in the code, it's not strictly a true __repr__, but that needs new code that we don't have yet and I'll argue should be another PR. Even this is still useful to have something for pdb to print out:

(Call StructuredName(add) [a, (Let a 2 2)])

makes equality of Expr's a lot easier to inspect than

<ksc.expr.Call object at 0x7fd1777058e0>
awf commented 3 years ago

Agree to not having repr use the parser. It should use the constructors. Can you show an example of how it doesn't currently?

acl33 commented 3 years ago

Agree to not having repr use the parser. It should use the constructors. Can you show an example of how it doesn't currently?

Here's a repr from the original post:

(Call StructuredName(add) [a, (Let a 2 2)])

So that would need to be...

Call(StructuredName.from_str("add"), [Var("a"), Let(Var("a"), Const(2), Const(2))])
awf commented 3 years ago

So that would need to be...

Call(StructuredName.from_str("add"), [Var("a"), Let(Var("a"), Const(2), Const(2))])

Yes, that seems more in the Python idiom?

acl33 commented 3 years ago

So that would need to be...

Call(StructuredName.from_str("add"), [Var("a"), Let(Var("a"), Const(2), Const(2))])

Yes, that seems more in the Python idiom?

Indeed. So do we think that we'd rather remove the repr here altogether? (In which case you'll get something like:

<ksc.expr.Call object at 0x7fd1777058e0>

)

acl33 commented 3 years ago

Agreed on repr. Also agreed that there is value in #926 - and I can see some value in sorting that out first, but since you have assigned #926 to yourself, thank you :), I'll merge this.