Sortinfo classes and smaller changes (for mapping)

[AlexKuhnle]

Changes:

• Sortinfo classes added (should be backward compatible, but let me know if not and why).
• Pred instantiation supported
• free_nodeid and visualise method added to Dmrs (and in serial)
• A few small improvements (hopefully)

[guyemerson]

For the Sortinfo class, I thought we agreed to keep the class mutable. If we inherit from namedtuple, it's immutable (and therefore not backward compatible).

Can I ask why we should support instantiating Preds directly, not as RealPreds or GPreds?

Visualisation code sounds like a good idea.

I'm not sure we need custom __copy__ and __deepcopy__ methods - I think it's safest to use the built-in python methods, unless we have a good reason to do otherwise.

Are there any other small changes that are worth drawing attention to?

[AlexKuhnle]

I fear I don't remember that we decided to keep it mutable. What would be the reasoning for having this one mutable, but not predicates (e.g. for mapping, I need mutability of both)? Anyway, I can add mutability, of course...

About Preds: I need an under-specified predicate value. I thought about just using None and removing this restriction, but I do the same for Sortinfo, and there None, i.e. non-existent, is different from underspecified, so I decided to make this distinction consistent.

I found the visualisation, although much less pretty than the demo website, quite useful since the demo website seems to be down (at least for me).

I added the copy and deepcopy methods because I ran into strange problems (can show you if you are interested - it threw exceptions from inside the Python copy library) for Sortinfo objects, which, I think, are related to the multiple inheritance with only one part being a tuple.

Small changes:

• I put "assert post" in the Link constructor (not for rargname because of None/EQ links). We can think of restricting to the allowed set of arguments (we mentioned last time that it's fixed), but not sure whether it's necessary.
• I changed assert "type(node) == self.Node" to "assert isinstance(node, self.Node)" where this was checked in Dmrs, to allow for e.g. my WildcardNodes (not every node in a graph then has the same class).
• Can't think of more...

One idea we could add: If a node is added with nodeid None, it could be assigned a nodeid using the free_nodeid method and then added.

[goodmami]

Sorry this is only tangentially related, but the demo site at NTU is down because of server issues they are having. I no longer have access to their servers so I cannot debug, but Francis and Luis will be attending to it soon. In the meantime, I setup a local instance at UW, although it only has the ERG 1214 and Jacy grammars: http://chimpanzee.ling.washington.edu/demophin

[guyemerson]

Okay, if we use an instance of Pred to mean an underspecified value, this needs to be clearly documented.

The reason I didn't think Preds needed to be mutable was that this would only be an issue if you wanted to change one of the slots of a RealPred (e.g. RealPred('cat','n','1') -> RealPred('cat','n','2')). This seems like a much smaller use case than using them in dicts or sets. Since Nodes are mutable, you can still change a node's pred, which I think is enough.

On the other hand, I imagine it might be much more common to change just one feature in Sortinfo (e.g. changing number of a noun or tense of a verb would be reasonable use cases). If Sortinfo is immutable, you have to construct a completely new object. I don't imagine SortInfo to be something people would want to hash - but do shout if there's a use case I've not thought of! (If there is, perhaps we need something like frozenset vs set)

[ Assert post in the Link constructor would actually be bad for me because I'm somewhat abusing the objects (using integer values for rargname, corresponding to indices of numpy arrays), but that's just me being naughty :P ]

[AlexKuhnle]

Alright, then we have Sortinfo mutable, and probably we should just get rid of the "assert post". Regarding the Pred issue: I should mention that for mapping Pred is an underspecified predicate, but I don't think this is something to put in the class description itself (since all the underspecified stuff only makes sense in the context of mapping, or otherwise one would want to include all of it in the main library).

[guyemerson]

It is absolutely something to put in the class description. If Pred instances are usable objects, then we need to document what they should be used for.

Underspecified preds may well be useful outside of mapping. But if it were only useful for mapping, then the mapping module should define its own class, rather than cluttering the core module.

[AlexKuhnle]

I can see that it is useful outside of mapping, but if we decide to put it in the core module, a few more methods might be of interest. For instance, I use '?' as an underspecified string for all the string arguments, and have a corresponding underspecifies method. Anyway, this can be discussed. I would not call it "cluttering the core module", since so far no real additional functionality is added, just the two NotImplementedErrors in str and repr removed - one was able to instantiate the Pred class before. I will upload an updated version soon...

[guyemerson]

I was actually considering throwing an error if you try to instantiate a Pred directly. I hadn't done this yet because it would mean either changing Pred to be an abstract base class (which RealPred and GPred are registered as subclasses of, rather than actually subclassing), or being more careful with how super is called (if Pred.__new__ throws an error, then RealPred.__new__ must avoid it).

Perhaps in the mean time, you should commit the uncontroversial changes (e.g. visualisation) and pend the rest until we can have a group discussion about these things.

[guyemerson]

Also, it might be a nice feature to let __lt__ and __gt__ depend on your subsumption checks.

[AlexKuhnle]

Yes, in case we include it in core, I would definitely put it in lt etc... And yes, since the changes are only in my fork, one can consider including them later.

[guyemerson]

Were the problems with copy and deepcopy just for Sortinfo or also for other classes? I'd be interested to see the error either way, but if it's for one of our existing classes, it would be great if you could open an issue for it.

[AlexKuhnle]

I think it had to do with the namedtuple thing in case of sortinfos. It doesn't happen when changing it to a mutable type (slots instead of namedtuple). So I will remove the copy/deepcopy methods again.