search

microsoft / knossos-ksc

Compiler with automatic differentiation
Other
45 stars 10 forks source link

Typing rewrites.py 3/n #974

Closed cgravill closed 2 years ago

cgravill commented 2 years ago

I've made it so you have to fully qualify typing.Type or ksc.type.Type as it leads to confusion.

@acl33 I've put in a assert isinstance(ewp.expr, Call) in inline_call which I'm not certain is correct. It passes tests but perhaps there are other types that should flow in here too?

Still a few classes of type error, particularly around the visitors and inheritance but we're getting there.

acl33 commented 2 years ago

I can see the visitor classes being awkward :(. I wonder if there's some other way to handle the args/kwargs that get passed through the superclass visit() onto the subclass visit_xxx(), or whether we have to figure out an appropriate #ignore, or something else.

This all looks good tho!

cgravill commented 2 years ago

I can see the visitor classes being awkward :(. I wonder if there's some other way to handle the args/kwargs that get passed through the superclass visit() onto the subclass visit_xxx(), or whether we have to figure out an appropriate #ignore, or something else.

This all looks good tho!

Great, thanks!

On the visitors. They are a bit problematic, going to take them one step at a time but I think we should get rid of the None return hint on the abstract methods:

https://github.com/microsoft/knossos-ksc/blob/96118312468d8affa1af4259bf9392777701bd1f/src/python/ksc/visitors.py#L35-L36

https://github.com/microsoft/knossos-ksc/blob/96118312468d8affa1af4259bf9392777701bd1f/src/python/ksc/rewrites.py#L489-L490

I get that they're methods that shouldn't be called, but marking them as None doesn't seem right to me or mypy.

cgravill commented 2 years ago

Anyway, think this is good progress and will merge. Shall return with 4/n another time.