delph-in / matrix

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

Fix for bug 672 #683

Closed KerenR3 closed 1 year ago

KerenR3 commented 1 year ago

Added missing SYNSEM to the head-adj-int-phrase rule in wh_ques.py

dantiston commented 1 year ago

Does this need an updated regression test? Since clearly there wasn’t a test before.

KerenR3 commented 1 year ago

I think there is an existing regression test that should have caught this problem, but the regression test passes even though bug 672 leads to a grammar that does not load in the lkb. I think this might be due to the fact that there are some redundant lines for the head-adj related rules across adverbs_adpositions.py and wh_ques.py. Getting rid of the redundant lines in adverbs_adpositions.py causes the regression test to fail unless bug 672 is fixed.

emilymbender commented 1 year ago

This doesn't make sense to me. If the grammar doesn't load, the regression test should be failing --- unless there's a larger bug in the regression testing system that counts tests as "passed" when there is no grammar.

Also, in what sense is the deleted material redundant? What error does the LKB give when trying to load such a grammar?

emilymbender commented 1 year ago

More broadly, I'd love to resolve whatever is behind requiring a separate set of head-adj rules (my-head-adj-phrase, etc). I think these have to do with @olzama 's wh questions library. Olga, do you remember what the motivation was for creating separate types? (At the very least, the name of the types should reflect that motivation, but I suspect we should be able to collapse them into the main types.)

olzama commented 1 year ago

I don't remember specifically. All I remember is I couldn't figure out how to use the existing types (I suppose trying to use them led to either compilation or regression test failures). But it was too long ago... This is the only mention that I found but it doesn't shed any light I'm afraid: https://github.com/delph-in/matrix/issues/589

dantiston commented 1 year ago

unless there's a larger bug in the regression testing system that counts tests as "passed" when there is no grammar.

I think the issue is that since the addition of ICONS, the regression testing system relies solely on ACE determining if a grammar compiles, loads, and parses.

emilymbender commented 1 year ago

That doesn't quite explain it though --- if the error that leads to the grammar not loading in the LKB also leads to the test passing, then there's more to it. Perhaps ace won't compile it either, and we're counting that as a "pass"?

KerenR3 commented 1 year ago

This doesn't make sense to me. If the grammar doesn't load, the regression test should be failing --- unless there's a larger bug in the regression testing system that counts tests as "passed" when there is no grammar.

Also, in what sense is the deleted material redundant? What error does the LKB give when trying to load such a grammar?

The error in LKB states that LOCAL is introduced at multiple types for HEAD-ADJ-INT-PHRASE. The line I deleted (within adverbs_adpositions.py) is a feature constraint (VAL [ SUBJ clist, COMPS clist ] ]) on my-head-adj-phrase. my-head-adj-phrase inherits from head-adj-int-phrase, and head-adj-int-phrase has this same feature constraint specified as an amendment to the matrix.py version within wh_ques.py. For the regression test itself, the test logs at least show that sentences are being parsed, even when the grammar won't compile in the LKB.

emilymbender commented 1 year ago

Hmm - what happens if you compile the grammar with ace? (Also, if you can, it would be great to have these grammars available to look at during Matrix/AGG in a few minutes.)

emilymbender commented 1 year ago

Just a few notes from our discussion today: 1) ace seems to be happily compiling a grammar with LOCAL introduced on two different types. Will investigate further. 2) that grammar passes the regression test, presumably because the extraneous constraint is ineffective (being in the wrong place in the tfs)