delph-in / pydmrs

A library for manipulating DMRS structures
MIT License
14 stars 6 forks source link

Errors when get_out/get_in used with nodes=True. #17

Closed emm68 closed 8 years ago

emm68 commented 8 years ago

The class Node is not hashable, so we can't return a set of nodes in get_out and get_in functions - error when attempted. I see two options:

  1. Change nodes to nodeids.
  2. Remove the option to return nodes. ?
guyemerson commented 8 years ago

Good point - I'd only used it with itr=True as well. So option 3 would be to force itr=True if nodes=True (and maybe give a warning).

I'm not sure about option 1 - I put the nodes parameter in so that a user could get to the interesting information faster, and nodeids aren't useful in themselves.

If we choose option 2, then I think we should put this functionality in another method.

AlexKuhnle commented 8 years ago

I would not mind returning nodeids, as I would assume that one often works with them instead of nodes anyway (and can still perform set operations). We could also return a list of nodes.

I would rather introduce a new method (neighbours?) than forcing itr to be True.

Another potentially useful aspect we don't use so far is that outgoing link labels, even rargname, are all unique, i.e. there is always only one ARG1 etc. I can see this to be useful (for incoming links the whole situation is different anyway).

guyemerson commented 8 years ago

If people think option 1 (returning nodeids) would be useful, we can add that.

Perhaps we could add get_out_nodes and get_in_nodes, which return lists rather than sets.

As for outgoing link labels being unique, this is true under MRS-wellformedness, but not under basic wellformedness (following the terminology in the LREC paper). So we'd have to be careful about how we approach that... Did you have a particular use case in mind?

emm68 commented 8 years ago

I don't have any use for nodeids other than get to the nodes itself, so in my opinion adding those two functions would be better.

On Wed, 16 Mar 2016 at 10:54 Guy Emerson notifications@github.com wrote:

If people think option 1 (returning nodeids) would be useful, we can add that.

Perhaps we could add get_out_nodes and get_in_nodes, which return lists rather than sets.

As for outgoing link labels being unique, this is true under MRS-wellformedness, but not under basic wellformedness (following the terminology in the LREC paper). So we'd have to be careful about how we approach that... Did you have a particular use case in mind?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/delph-in/pydmrs/issues/17#issuecomment-197260120

AlexKuhnle commented 8 years ago

You're right about the wellformedness, maybe better to not add it to not give wrong ideas. The node ids are definitely useful, e.g. for the disconnected algorithm. And since the algorithm would use the node ids in link.start/end to get to the nodes anyway, I would definitely support it.

emm68 commented 8 years ago

Sorry, I clicked close, instead of comment and close. I added the new functions for returning nodes and got rid of nodes parameter in get_in/get_out. I did not include an option for nodeids, figuring t's easy to add it if we do want it.

guyemerson commented 8 years ago

Thanks. There are a few typos, but I'll fix them now.

emm68 commented 8 years ago

Ouch, sorry, I just noticed them too. I hope I got them all with the fix.

guyemerson commented 8 years ago

Looks good to me.