-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
interpretation of TOP property #25
Comments
Good catch. Here is an even-more-minimal test case: $ cat i25a
(a / alpha
:ARG0 (b / beta))
$ cat i25b # only top concept different
(a / alternative
:ARG0 (b / beta))
$ cat i25c # only non-top concept different
(a / alpha
:ARG0 (b / b-side))
$ python smatch.py --pr -f i25a i25b
Precision: 0.50
Recall: 0.50
F-score: 0.50
$ python smatch.py --pr -f i25a i25c
Precision: 0.75
Recall: 0.75
F-score: 0.75 When smatch is run with
Note that the concept --- a/amr.py
+++ b/amr.py
@@ -418,7 +418,7 @@ class AMR(object):
relation_list.append(node_rel_list)
attribute_list.append(node_attr_list)
# add TOP as an attribute. The attribute value is the top node value
- attribute_list[0].append(["TOP", node_value_list[0]])
+ relation_list[0].append(["TOP", node_name_list[0]])
result_amr = AMR(node_name_list, node_value_list, relation_list, attribute_list)
return result_amr Results in: $ python smatch.py --pr -f i25a i25b
Precision: 0.75
Recall: 0.75
F-score: 0.75 Now it gets the score of 0.75 for both when the top label is different (
|
i believe it has not consequences for the overall F1 score, but in my view the TOP property would better be considered a node attribute rather than a relation between a node and itself. treating it as an attribute would avoid (vacuously) stipulating a self-loop in the graph, and it preserves a one-to-one correspondence between relation triples and actual graph edges. |
If that matters to you, then here's a diff that creates an attribute triple instead. It is probably equally unlikely to be confused with an actual graph triple (since we have no role called --- a/amr.py
+++ b/amr.py
@@ -421,7 +421,7 @@ class AMR(object):
relation_list.append(node_rel_list)
attribute_list.append(node_attr_list)
# add TOP as an attribute. The attribute value is the top node value
- attribute_list[0].append(["TOP", node_value_list[0]])
+ attribute_list[0].append(["TOP", 'top'])
result_amr = AMR(node_name_list, node_value_list, relation_list, attribute_list)
return result_amr |
hi again, @goodmami: the above looks just right to me (treating the TOP property as an attribute with a vacuous value). would you be set up to submit that as a pull request? i am wondering whether we could try to work through some more of the known issues and hope to arrive at a re-release of an improved SMATCH sometime over the next few weeks? |
Yes, I can put together a PR for that, but for a re-release what do people think of replacing |
* Add tests/ subdirectory with one test file * Make TOP relation value constant Fixes #25 * Clarify CHANGELOG entry
when comparing SMATCH results with the scorer from the 2019 CoNLL Shared Task on Cross-Framework Meaning Representation Parsing (MRP), we discovered that SMATCH will only consider the TOP property correct if the node labels also match. this appears to double-penalize for label mismatches and is maybe not the intended behavior? for more technical detail and a minimal test case, please see the MRP
mtool
issue.The text was updated successfully, but these errors were encountered: