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?
There was a problem hiding this comment.
Is this the correct name for what's in branch targets?
I believe this would be Atom. Considering the grammar, atoms are variables or constants, constants are either strings or symbols. Here's an example:
(a / A
:ARG1 (b / B) ; node target
:ARG2 b ; variable target (atomic)
:ARG3 "a b" ; string target (atomic)
:ARG4 abc ; symbol target (atomic)
)The only difference between a symbol and a variable is that a variable is used in a node (such as a and b in the example above). The difference between strings and symbols, besides the quotes, is just that symbols cannot contain control characters like whitespace, parens, colons, etc. Quoted strings may not be used as variables (("a" / A) raises an error, and :ARG1 "a" does not reference a variable a).
even number constants showing up as strings
That is correct. Penman does no interpretation of datatypes on parsing. It will accept a limited number of non-string types (ints, floats, etc.) during encoding, but they will be strings again when decoding.
Are there edge-cases of trees where this isn't correct?
While unconventional, missing targets are allowed and parse to None (a warning may be issued as well):
>>> penman.parse('(a / A :ARG1)')
Missing target: (a / A :ARG1)
Tree(('a', [('/', 'A'), (':ARG1', None)]))Similarly, an empty node target means that the type annotation of Node is not entirely correct, as mentioned in #129:
>>> penman.parse('(a / A :ARG1 ())')
Tree(('a', [('/', 'A'), (':ARG1', (None, []))]))
| # 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?
There was a problem hiding this comment.
Probably not for AMR, but it is a possible graph for this Penman library:
>>> penman.parse('(a)') # tree node has empty branch list
Tree(('a', []))
>>> penman.decode('(a)').triples # graph has :instance None
[('a', ':instance', None)]
| # 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 🤷♂️
There was a problem hiding this comment.
Yeah this is a little awkward. In general, I'm not too concerned about reducing performance as long as it's correct.
I haven't looked closely at this code in a while so I'd need to think a bit more about a better solution, but in the meantime I want to point out that the change from else to elif means that there is no more else case. A reader of the code would have to know that is_tgt_symbol() and is_tgt_node() are defined as opposites to determine that the elif would catch all other cases. Otherwise it looks like a latent bug.
goodmami
left a comment
goodmami
left a comment
There was a problem hiding this comment.
Apologies for the delayed response. I have some comments below which would require some changes, but I don't have specific suggestions. Hopefully they are informative for you nonetheless.
| 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?
I believe this would be Atom. Considering the grammar, atoms are variables or constants, constants are either strings or symbols. Here's an example:
(a / A
:ARG1 (b / B) ; node target
:ARG2 b ; variable target (atomic)
:ARG3 "a b" ; string target (atomic)
:ARG4 abc ; symbol target (atomic)
)The only difference between a symbol and a variable is that a variable is used in a node (such as a and b in the example above). The difference between strings and symbols, besides the quotes, is just that symbols cannot contain control characters like whitespace, parens, colons, etc. Quoted strings may not be used as variables (("a" / A) raises an error, and :ARG1 "a" does not reference a variable a).
even number constants showing up as strings
That is correct. Penman does no interpretation of datatypes on parsing. It will accept a limited number of non-string types (ints, floats, etc.) during encoding, but they will be strings again when decoding.
Are there edge-cases of trees where this isn't correct?
While unconventional, missing targets are allowed and parse to None (a warning may be issued as well):
>>> penman.parse('(a / A :ARG1)')
Missing target: (a / A :ARG1)
Tree(('a', [('/', 'A'), (':ARG1', None)]))Similarly, an empty node target means that the type annotation of Node is not entirely correct, as mentioned in #129:
>>> penman.parse('(a / A :ARG1 ())')
Tree(('a', [('/', 'A'), (':ARG1', (None, []))]))| # Tree types | ||
| Branch = Tuple[Role, Any] | ||
| Branch = Tuple[Role, Union[Symbol, "Node"]] | ||
| Node = Tuple[Variable, List[Branch]] |
There was a problem hiding this comment.
Probably not for AMR, but it is a possible graph for this Penman library:
>>> penman.parse('(a)') # tree node has empty branch list
Tree(('a', []))
>>> penman.decode('(a)').triples # graph has :instance None
[('a', ':instance', None)]| def is_tgt_node(target: Symbol | Node) -> TypeGuard[Node]: | ||
| """ | ||
| Inverse of :func:`is_atomic`, only for Symbol | Node from branches. | ||
| Automatically narrows the type to Node for better type inference | ||
| """ | ||
| return not is_atomic(target) | ||
|
|
||
|
|
||
| def is_tgt_symbol(target: Symbol | Node) -> TypeGuard[Symbol]: | ||
| """ | ||
| Same as :func:`is_atomic`, only for Symbol | Node from branches. | ||
| Automatically narrows the type to Symbol for better type inference | ||
| """ | ||
| return is_atomic(target) |
There was a problem hiding this comment.
There are a few issues here:
- It's not great that these don't really do anything different from
is_atomic()except for type checking. I think having them as part of the public API would confuse users. - If they were to stay, I'd prefer to use non-abbreviated names in public API functions:
is_target_symbol()or maybeis_symbol(). TypeGuardis added in Python 3.10, but Penman currently supports down to Python 3.8.
| # nested nodes | ||
| else: | ||
| # mypy forgets that (Node ∨ Sym) ^ ¬Sym → Node | ||
| elif is_tgt_node(target): |
There was a problem hiding this comment.
Yeah this is a little awkward. In general, I'm not too concerned about reducing performance as long as it's correct.
I haven't looked closely at this code in a while so I'd need to think a bit more about a better solution, but in the meantime I want to point out that the change from else to elif means that there is no more else case. A reader of the code would have to know that is_tgt_symbol() and is_tgt_node() are defined as opposites to determine that the elif would catch all other cases. Otherwise it looks like a latent bug.
2 participants
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.