Add interface for defining external statistics. #23

BenKaufmann · 2018-03-25T17:51:00Z

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().

* Single-thread builds must not (yet) use C++11 features.

rkaminsk · 2018-03-25T18:06:43Z

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

Looks good, thanks for your effort.

pluehne · 2018-03-26T09:31:57Z

CHANGES

@@ -1,5 +1,6 @@
 clasp 3.3.4:
  * fixed: Atoms of incremental programs not always marked as frozen.
+  * added internal interface for user defined statistics


For consistency, I’d write “user-defined“ with a hyphen here as well.

Yep. Thanks.

pluehne · 2018-03-26T09:34:19Z

src/clasp_facade.cpp

 	if (f.incremental()) {
 		accu_ = new Accu();
 		accu_->step.bind(*f.accu_.get());
 	}
 	setRoot(keys_.toStats());
 }
 void ClaspFacade::Statistics::ClingoView::update(const ClaspFacade::Statistics& stats) {
+	if (stats.userStats_.get() && !stats.userStats_->empty()) {
+		keys_.add("userdefined", stats.userStats_->toStats());


Is there a reason why userdefined is spelled without a hyphen here? I know that comment is a bit picky, but it might be inconvenient to make such things consistent at a later point in time.

For consistency, this should be user_defined. Or alternatively, external to indicate that these are not maintained by clasp.

Okay. I was just referring to the fact that “user-defined” is a compound adjective and should be written with a hyphen, but if hyphens are not allowed within keys, that’s just how it is 🙂.

Sorry, I forgot to add the "Yep, you are right". I just wanted to express that our keys typically use underscores (and unfortunately sometimes camel-case) instead of hyphens.

I totally understand, thanks for the info … and sorry about the fuss and my pedantry anyway 😄!

MaxOstrowski · 2018-04-16T12:51:34Z

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 · 2018-04-16T13:53:32Z

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

BenKaufmann · 2018-04-22T07:55:59Z

@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 · 2018-04-27T11:40:57Z

@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 · 2018-04-29T15:39:37Z

@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 · 2018-05-02T12:48:40Z

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 · 2018-05-02T17:23:41Z

@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 · 2018-05-03T07:33:52Z

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

rkaminsk · 2018-05-03T08:22:04Z

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 · 2018-05-04T18:34:54Z

@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 · 2018-05-04T19:22:44Z

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 · 2018-05-05T09:14:28Z

@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 · 2018-05-05T18:55:02Z

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

rkaminsk · 2018-05-06T08:28:53Z

Thanks, was too slow to agree too. 😉

BenKaufmann added 2 commits March 25, 2018 19:10

Add interface for defining external statistics.

e916db3

Fix warnings and errors in single-thread build.

ac6941d

* Single-thread builds must not (yet) use C++11 features.

BenKaufmann requested review from rkaminsk and MaxOstrowski March 25, 2018 17:51

BenKaufmann mentioned this pull request Mar 25, 2018

added userdefined statistics #20

Closed

MaxOstrowski approved these changes Mar 26, 2018

View reviewed changes

pluehne reviewed Mar 26, 2018

View reviewed changes

Address review comments and minor cleanup.

58748a4

Merge ExternalStatistics into AbstractStatistics.

6865b14

BenKaufmann mentioned this pull request Apr 22, 2018

External stats change #26

Closed

Fix broken commit.

8f57a3f

Switch to push-based AbstractStatistics array API.

4a72e0f

Implement AbstractStatistics::find().

4743d55

BenKaufmann merged commit d9862b9 into dev May 6, 2018

BenKaufmann deleted the external_stats branch May 6, 2018 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interface for defining external statistics. #23

Add interface for defining external statistics. #23

BenKaufmann commented Mar 25, 2018

rkaminsk commented Mar 25, 2018

MaxOstrowski left a comment

pluehne Mar 26, 2018

BenKaufmann Mar 26, 2018

pluehne Mar 26, 2018

BenKaufmann Mar 26, 2018

pluehne Mar 26, 2018 •

edited

Loading

BenKaufmann Mar 26, 2018 •

edited

Loading

pluehne Mar 26, 2018

MaxOstrowski commented Apr 16, 2018

MaxOstrowski commented Apr 16, 2018 •

edited

Loading

BenKaufmann commented Apr 22, 2018

MaxOstrowski commented Apr 27, 2018

BenKaufmann commented Apr 29, 2018

MaxOstrowski commented May 2, 2018

BenKaufmann commented May 2, 2018 •

edited

Loading

MaxOstrowski commented May 3, 2018

rkaminsk commented May 3, 2018

BenKaufmann commented May 4, 2018

rkaminsk commented May 4, 2018

BenKaufmann commented May 5, 2018

MaxOstrowski commented May 5, 2018

rkaminsk commented May 6, 2018

Add interface for defining external statistics. #23

Add interface for defining external statistics. #23

Conversation

BenKaufmann commented Mar 25, 2018

rkaminsk commented Mar 25, 2018

MaxOstrowski left a comment

Choose a reason for hiding this comment

pluehne Mar 26, 2018

Choose a reason for hiding this comment

BenKaufmann Mar 26, 2018

Choose a reason for hiding this comment

pluehne Mar 26, 2018

Choose a reason for hiding this comment

BenKaufmann Mar 26, 2018

Choose a reason for hiding this comment

pluehne Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

BenKaufmann Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

pluehne Mar 26, 2018

Choose a reason for hiding this comment

MaxOstrowski commented Apr 16, 2018

MaxOstrowski commented Apr 16, 2018 • edited Loading

BenKaufmann commented Apr 22, 2018

MaxOstrowski commented Apr 27, 2018

BenKaufmann commented Apr 29, 2018

MaxOstrowski commented May 2, 2018

BenKaufmann commented May 2, 2018 • edited Loading

MaxOstrowski commented May 3, 2018

rkaminsk commented May 3, 2018

BenKaufmann commented May 4, 2018

rkaminsk commented May 4, 2018

BenKaufmann commented May 5, 2018

MaxOstrowski commented May 5, 2018

rkaminsk commented May 6, 2018

pluehne Mar 26, 2018 •

edited

Loading

BenKaufmann Mar 26, 2018 •

edited

Loading

MaxOstrowski commented Apr 16, 2018 •

edited

Loading

BenKaufmann commented May 2, 2018 •

edited

Loading