Fix typing in multiple places #7333
Conversation
| @@ -28,7 +28,7 @@ | |||
| from pytensor import tensor as pt | |||
| from pytensor.compile.builders import OpFromGraph | |||
| from pytensor.graph import FunctionGraph, clone_replace, node_rewriter | |||
| from pytensor.graph.basic import Node, Variable, io_toposort | |||
| from pytensor.graph.basic import Apply, Variable, io_toposort | |||
There was a problem hiding this comment.
Only comment: it's a bit awkward to have variables called node typed as Apply instead of Node. I was thinking you could do an alias here: from pytensor.graph.basic import Apply as ApplyNode to make the type hints a bit more obvious? Or maybe that's a name change that we should consider at the pytensor level @ricardoV94 . It would match Variable -> TensorVariable that way.
There was a problem hiding this comment.
Node is a bit of useless baseclass for Apply and Variable. The only thing shared between the two is the get_parents. We definitely like to call Apply(Node) nodes in the codebase, and never corresponding Variable(Node) nodes, or variable nodes. I think this gives us so little that we could remove the shared base class and only have Variables and Nodes (although I understand why they wanted to call a Variable a node as well). Anyway that should be a discussion on the PyTensor side, I don't think PyMC should invent new names here.
There was a problem hiding this comment.
Yeah I agree, I'll raise an issue on the pytensor side about typing generally. @michaelosthege please ignore my comment, PR looks good to go
There was a problem hiding this comment.
Thanks for the explanation. I agree that things should be improved on the PyTensor side. The type hinting is a bit of a mess down there too.
Description
Fixes to type hints, mostly in
printing.pyandlogprob/order.py.Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7333.org.readthedocs.build/en/7333/