delph-in / matrix

The Grammar Matrix
https://matrix.ling.washington.edu/index.html
Other
12 stars 6 forks source link

Fix 627 #647

Closed ltxom closed 2 years ago

ltxom commented 2 years ago

This PR fixes issue #627 .

There are two improvements:

  1. validation reports an error if a verb inherits another verb and has valence. (lexicon.py line #445-#452)
  2. validation shows a question mark to warn the user of other duplicated features (maybe different values) with inheritance hierarchy.

For the second improvement, there are existing codes that are implemented for nouns, but this functionality is tangled with only 'det' and 'noun' POS.

Therefore, I created a new section and a helper method to check for other POS.

Regression Tests are all passed after these improvements. Here's a choices file to test (demo in https://matrix.ling.washington.edu/ltxom-test/)

emilymbender commented 2 years ago

Thank you for this! I'd like to suggest a change to the validation message. Instead of:

mess = 'You must either remove the valence of a subtype verb ' \ 'or remove the inheritance hierarchy for this verb.'

I propose:

mess = 'A verb class that specifies a value for valence can't also inherit a value for valence.'

Regarding the warning for the noun, it's a bit surprising that it shows up next to "add a feature". Is it possible to have the red ? show up next to the feature/value pair instead?

ltxom commented 2 years ago

Thank you for your feedback! I updated the error message and moved the noun's warning next to the feature/value pair as you suggested.

emilymbender commented 2 years ago

Looking better! I want to suggest a rephrasing for the warning on the nouny features. Instead of:

mess = 'Please be aware that this type has a duplicated feature (' + \ lt.full_key + ': ' + feat.full_key + \ ') with inheritance hierarchy, ' \ 'which can be factors to create redundant or conflicting grammars.'

How about:

mess = 'A value for this feature is already specified on a supertype for this noun class. If the value specified here is conflict with that other specification, the grammar will not compile.'

The identifier of the feature doesn't need to be included in the message, because the position of the red ? makes it clear which feature is at issue.

ltxom commented 2 years ago

Thank you for the suggestion! Please check it now :)

emilymbender commented 2 years ago

Missing a space between "other" and "specification" but otherwise looks good!

ltxom commented 2 years ago

Oops, my fault. It should be good now.

emilymbender commented 2 years ago

Looks good! Merged.