Adding typing to tree branches #139
Conversation
|
|
||
| Variable = str | ||
| Constant = Union[str, float, int, None] # None for missing values | ||
| Role = str # '' for anonymous relations | ||
| Symbol = str |
There was a problem hiding this comment.
Is this the correct name for what's in branch targets? It seemed like the target must always be a string if it's not a Node, with even number constants showing up as strings. Are there edge-cases of trees where this isn't correct?
|
|
||
| # Tree types | ||
| Branch = Tuple[Role, Any] | ||
| Branch = Tuple[Role, Union[Symbol, "Node"]] | ||
| Node = Tuple[Variable, List[Branch]] |
There was a problem hiding this comment.
Is it possible to have an AMR tree that consists of just a single Variable with no branches?
| @@ -178,7 +178,8 @@ def _interpret_node(t: Node, variables: Set[Variable], model: Model): | |||
| triples.append(triple) | |||
| epidata.append((triple, epis)) | |||
| # nested nodes | |||
| else: | |||
| # mypy forgets that (Node ∨ Sym) ^ ¬Sym → Node | |||
| elif is_tgt_node(target): | |||
There was a problem hiding this comment.
This is pretty awkward, and will technically reduce performance just to get typing to work. Annoyingly, Mypy Typeguard doesn't work the same as isinstance() (python/typing#1351). So, replacing is_tgt_symbol(target) above with isinstance(target, str) then allows Mypy to infer that it must a Node here and a Symbol above without needing an elif here. Not sure which solution is best, since the method is more clear what's going on compared to a naked isinstance.
Alternatively, we could just use cast to force Mypy to recognize the correct type 🤷♂️
This PR removes the
Anyfrom theBranchtype, so trees are fully recursively typed.This is a first step towards #129, but figured this would be a small enough self-contained chunk of work to check if this is on the right path.