Add interface for defining external statistics.

[BenKaufmann]

Dear Max,

I created an alternative implementation for your pull request #20 that is a little bit more in the spirit of the existing clasp code.

Please let me know if this interface suits your needs or if you have any comments. Given that this new interface is quite low-level and not type-safe, I deliberately used more explicit function names, like e.g. mapAdd() and arrayAdd().

[rkaminsk]

I'll talk to Max. It is probably a good idea to try to use this for the statistics in clingo and give feedback based on this. -R

[MaxOstrowski]

Unused array indices are filled with empty objects. Example: https://github.com/MaxOstrowski/clasp/tree/external_stats Is it possible to make them of type empty, as the clingo interface supports this statistics_type?

[MaxOstrowski]

I will add a pull request regarding a unified interface for reading tomorrow.

[BenKaufmann]

@MaxOstrowski I addressed the two issues you raised, i.e. I unified the interface (this also includes a new branch in libpotassco) and changed the behaviour of add() for arrays.

• Regarding array add(): holes are now filled with objects of type Empty, which can later be overwritten. I do not particular like this change because previously Empty was only used to signal errors (in places where exceptions are undesirable). Maybe we could switch to a less general push(...) interface that only allows for appending single objects thereby preventing the creation of holes in the first place.
• For integrating the stats interfaces, I introduced a new writable property indicating whether calling add on a key is allowed. Starting from a writable root, new writable keys can be added but all existing clasp stats are considered read-only.

I'm not sure whether I got the interplay between the "user root" and the stats callbacks right. The current implementation simply calls the callbacks with a stats object that is rooted at "user_stats". That is, inside of callbacks, clasp statistics are hidden and new stats automatically occur under "user_stats". However, alternative designs are certainly possible:

• We could pass the full stats object (including the clasp stats) and either add a new parameter userRoot of type Key_t to the callbacks or add a function userRoot() to the AbstractStatistics interface.
• We could get rid of the notion of a dedicated user root and instead let callbacks add their own root keys. To handle output in clasp, we could then either limit printing to some dedicated key (e.g. "user_stats") or go the whole nine yards and print all user roots.

Any preferences?

[MaxOstrowski]

@BenKaufmann Thanks a lot for all your efforts. I talked to Roland and he liked the idea of the push interface, which should be enough for handling all use cases. The writeable flag is good for the clasp interface. You understood the intention of user_root correctly. The plan is to make the statistics object const in the old interface and non-const for the callbacks. Also user_root should be used during the callbacks.

I'm not sure whether I got the interplay between the "user root" and the stats callbacks right. The current implementation simply calls the callbacks with a stats object that is rooted at "user_stats". That is, inside of callbacks, clasp statistics are hidden and new stats automatically occur under "user_stats".

This is perfectly fine

[BenKaufmann]

@MaxOstrowski I implemented the push() based array interface and updated the PR accordingly. If you are happy with the current version, I'll merge the PR to dev. Otherwise, let me know what is missing.

[MaxOstrowski]

Looks nice, thank you very much. Roland requested a has_subkey function that I'm not able to implement. Would it be possible to add such a thing ?

[BenKaufmann]

@MaxOstrowski Can you explain a little bit how such a has_subkey() function should look like? Do you mean something along the lines of virtual bool tryGet(Key_t mapK, const char* key, Key_t* outKey) const;, i.e. a function that can be used to query for the existence of a named statistic object?

[MaxOstrowski]

Exactly. This function allows Roland to implement any clingo interface he wants. Thanks a lot.

[rkaminsk]

Testing for the existence of a key in a map is a standard operation on maps in one of our target API languages: python. So far the statistics object in the python API was simply a python dict(). This is no longer possible once we make it writable (at least if we want a nice API). In python there are the Mapping and Sequence protocols. As far as I can see only a way to query for a subkey is missing to implement them (well, with the current API it could be implemented in linear time). This way the API can present nice pythonic interfaces that won't surprise our users.

Ben's tryGet function proposal should do the job.

[BenKaufmann]

@rkaminsk I understand your point but just to clarify: The suggested tryGet() will also have linear complexity and at least for non-writable keys it will even have to apply a try/catch-based search, where non-existence is internally communicated via an exception. So, tryGet() will have ok performance in the best and pretty horrible performance in the worst case (if we assume a table-based approach to exceptions).

[rkaminsk]

I do not care much for performance with the statistics. This is not a critical code point. And since it will be hidden behind an interface it can be changed if it becomes a problem.

[BenKaufmann]

@rkaminsk @MaxOstrowski I added bool AbstractStatistics::find(Key_t mapK, const char* key, Key_t* outKey). Anything else missing or should I merge to dev?

[MaxOstrowski]

You can merge with dev, erverything should work now. Thanks a lot !

[rkaminsk]

Thanks, was too slow to agree too. :wink: