search

cu-clear / semlink

Official repository for Semlink resources
32 stars 10 forks source link

PB-VN mappings have been re-broken #6

Closed aaronstevenwhite closed 3 years ago

aaronstevenwhite commented 3 years ago

In March, I pointed out to @ghamzak in an email discussion that the PB-VN mapping were treated as one-to-one but that they must be one-to-many because the same PB roleset can map to distinct VN classes. This commit fixed the problem, but the error was reintroduced by this commit (@kevincstowe). I might suggest developing a JSON schema against which these mappings are validated prior to merging into master to avoid reintroducing this error in the future.

kevincstowe commented 3 years ago

Unfortunately the prior commit also deleted a bunch of the mappings, hence the error introducing fix. I've reworked the script so it now generates in a new format which captures the one-to-many mapping:

{"accept.01": {"29.2-1-1": {"ARG0": "agent", "ARG1": "theme", "ARG3": "attribute"}, "77.1": {"ARG0": "agent", "ARG1": "theme"}, "13.5.2": {"ARG0": "agent", "ARG1": "theme", "ARG2": "source"}}}

The keys remain the same. The value is a dictionary keyed by VN classes, with the values being the respective argument mappings. Personally I think this is a bit cleaner, but I'm also happy with the previous formatting, with the value being a list of dictionaries with "vn_class" and "args" attributes. Which would you prefer?

aaronstevenwhite commented 3 years ago

The above looks great. Thank you.

kevincstowe commented 3 years ago

Okay great, I've pushed those and updated the generator. We still need to look into some kind of validation to ensure everything's valid as you suggested, but for now these should be good.

aaronstevenwhite commented 3 years ago

Great. Thanks so much for the quick reply.