Weird comma swapping in demo

[matt-gardner]

Noticed in the demo @codeviking gave today: http://demo.allennlp.org/semantic-role-labeling/NTc1OA==. The comma gets moved to after "which" in the heirplane visualization. This doesn't happen in the text visualization.

[matt-gardner]

Interestingly, also, this only happens on the first verb - when you click over to the second verb, the comma goes back to where it should be.

[codeviking]

Interesting. Punctuation handling in hierplane is a bit tricky. I'll take a look first thing on Monday. Thanks!

[schmmd]

@codeviking hi Sam, this isn't urgent (and doesn't need attention) but I'm pinging you in case you forgot to look at this yet want to.

[codeviking]

Doh. I did.

It'd likely be good to have @aaronsarnat take a look, since he's officially taking over Hierplane. @aaronsarnat would you have time before sabbatical to take a look?

[codeviking]

Also, @schmmd I can't for some reason assign @aaronsarnat, so if that works for you (and Aaron) let's swap it into his court.

[aaronsarnat]

My pre-sabbatical responsibilities have expanded recently, so give me a few days to follow up on this.

[codeviking]

Fantastic, thanks @aaronsarnat !

[aaronsarnat]

Sorry, what I meant by "follow up" is I need to check w/ Mark H. about this ask relative to my other priorities which have increased in scope this month. I want to help if I can, but am already on borrowed time. Will circle back here tomorrow after I've had my 1:1 w/ Mark.

[aaronsarnat]

@codeviking when this came up before, had you by chance done any investigation yet to get a sense for the nature of the problem or what kind of time might be involved in addressing it? If so, this might be helpful to know when I'm talking w/ Mark.

[codeviking]

No clue, unfortunately.

The way punctuation is handled is currently pretty hacky, as you know. It could be a straightforward fix -- it also could be as simple as an issue with the data (I'd start by inspecting the returned parses and see if the comma is in fact being put in a different location).

If it's related to the data, it's super easy, in that you just have to note this and punt it back to Michael :). If it's not that, we'll have to assess how hard it'll be to fix -- and it might be "won't fix" for now given the state of how the lib under the hood handles punctuation.

[DeNeutoy]

I think this is caused by the way we are adding reference nodes to spans. In the example Matt gave above, [The keys] is a span, which then gets a Node added to it by [which], which is rendered before the ignoredSpans from the [The keys] Node, which (hopefully) includes the comma. I'm not exactly clear on how the ignoredSpans are rendered with respect to the text from a Node though.

[aaronsarnat]

Hey guys, sorry I haven't followed up yet. Mark H. is out of office this week, so haven't caught up with him yet about this. Briefly chatted w/ Michael and it sounds like this issue is not a super high priority. I've got this item on my "nice to have" list and would like to help if I can, but need to crank through some of the bigger ticket items on my list first before I go on break.

I will check back in on this in the coming weeks to see how things are shaping on on my end.

And thanks @DeNeutoy for the investigation!

[matt-gardner]

Closing this in favor of the issue on the demo repo.