Adding typing to tree branches

[chanind]

This PR removes the Any from the Branch type, 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.

[chanind]

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?

[goodmami]

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, []))]))

[chanind]

Is it possible to have an AMR tree that consists of just a single Variable with no branches?

[goodmami]

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)]

[chanind]

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 🤷‍♂️

[goodmami]

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]

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.

[goodmami]

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, []))]))

[goodmami]

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)]

[goodmami]

There are a few issues here:

1. 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.
2. If they were to stay, I'd prefer to use non-abbreviated names in public API functions: is_target_symbol() or maybe is_symbol().
3. TypeGuard is added in Python 3.10, but Penman currently supports down to Python 3.8.

[goodmami]

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.