nschneid / amr-hackathon

Abstract Meaning Representation (AMR) Hackathon
MIT License
28 stars 11 forks source link

negative values not allowed by parser #2

Closed jonmay closed 7 years ago

jonmay commented 8 years ago

as indicated in the recent pull request, the following valid amr:

( o / have-01 :ARG0 o :value -2)

is not liked by the rules. The error messages that are thrown are not directly related to the problem, which seems to be bad regexes.

nschneid commented 8 years ago

Good catch. I think it's not the fault of the regex:

NUM = ~r"[+-]?\d+(\.\d+)?" ALIGNMENT?

should match negative numbers in principle. I bet it's due to the fact that + or - by itself can be a constant, e.g. in :polarity -, and the line

Y = X / NAMEDCONST / VAR / STR / NUM

tries matching a + or - constant before it tries matching a number. You could try fiddling with the order of that line (putting NUM before NAMEDCONST). If it causes other problems it might be necessary to split up NAMEDCONST, which would require some parsing code changes.

jonmay commented 8 years ago

thanks! will try.

nschneid commented 8 years ago

@jonmay, any luck?

jonmay commented 8 years ago

it fixed some problems but caused others. I realized a workaround that better targeted what I was trying to do: now the amr validator uses smatch's own amr reading library to verify the amr is valid (at least according to smatch)

On Mon, Nov 30, 2015 at 3:41 PM, nschneid notifications@github.com wrote:

@jonmay https://github.com/jonmay, any luck?

— Reply to this email directly or view it on GitHub https://github.com/nschneid/amr-hackathon/issues/2#issuecomment-160799893 .

"Je n’ai fait celle-ci plus longue que parce que je n’ai pas eu le loisir de la faire plus courte." -- Pascal

jonmay commented 8 years ago

actually, you may want to look at the amr.py code released with smatch. it's not super elegantly written but it does seem fairly robust.

On Mon, Nov 30, 2015 at 4:42 PM, Jon May jonmay@gmail.com wrote:

it fixed some problems but caused others. I realized a workaround that better targeted what I was trying to do: now the amr validator uses smatch's own amr reading library to verify the amr is valid (at least according to smatch)

On Mon, Nov 30, 2015 at 3:41 PM, nschneid notifications@github.com wrote:

@jonmay https://github.com/jonmay, any luck?

— Reply to this email directly or view it on GitHub https://github.com/nschneid/amr-hackathon/issues/2#issuecomment-160799893 .

"Je n’ai fait celle-ci plus longue que parce que je n’ai pas eu le loisir de la faire plus courte." -- Pascal

"Je n’ai fait celle-ci plus longue que parce que je n’ai pas eu le loisir de la faire plus courte." -- Pascal

nschneid commented 8 years ago

actually, you may want to look at the amr.py code released with smatch.

Is that @ulfulf's recent rewrite?

jonmay commented 8 years ago

yes(?) not sure how much of the core amr.py he touched though; it doesn't look like his style.

On Mon, Nov 30, 2015 at 4:44 PM, nschneid notifications@github.com wrote:

actually, you may want to look at the amr.py code released with smatch.

Is that @ulfulf https://github.com/ulfulf's recent rewrite?

— Reply to this email directly or view it on GitHub https://github.com/nschneid/amr-hackathon/issues/2#issuecomment-160810439 .

"Je n’ai fait celle-ci plus longue que parce que je n’ai pas eu le loisir de la faire plus courte." -- Pascal

nschneid commented 8 years ago

The parsing code looks very well documented, which is Ulfish. :) Without looking at it in detail, I suspect it's preferable to mine because (a) it may be more robust and (b) it is written from scratch, and therefore doesn't depend on external libraries.

However, I think my API provides additional functionality that would be nice to have. I'd be happy to work on merging the two together once @ulfulf posts the code on GitHub (so we avoid the specter of versioning difficulties).

uhermjakob commented 8 years ago

I have so far not uploaded any Smatch/AMR code to github, and if I looked at the right amr.py it's not mine. I tried the example with the negative value on all ISI Smatch scores and it worked fine on all versions (1.0, 2.0, 2.1). More later.

jonmay commented 8 years ago

yeah, sounds like we're talking about two different versions. i don't recall seeing anything that was well documented. the amr.py i'm referring to is included in my 'validator' code:

http://alt.qcri.org/semeval2016/task8/data/uploads/validator.tar.gz

On Mon, Nov 30, 2015 at 5:23 PM, Ulf Hermjakob notifications@github.com wrote:

I have so far not uploaded any Smatch/AMR code to github, and if I looked at the right amr.py it's not mine. I tried the example with the negative value on all ISI Smatch scores and it worked fine on all versions (1.0, 2.0, 2.1). More later.

— Reply to this email directly or view it on GitHub https://github.com/nschneid/amr-hackathon/issues/2#issuecomment-160816659 .

"Je n’ai fait celle-ci plus longue que parce que je n’ai pas eu le loisir de la faire plus courte." -- Pascal

danielhers commented 7 years ago

Fixed.