Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added userdefined statistics #20

Closed
wants to merge 1 commit into from
Closed

added userdefined statistics #20

wants to merge 1 commit into from

Conversation

MaxOstrowski
Copy link
Member

added the option to add userdefined statistics to be set from outside, e.g. clingo scripts

Copy link
Contributor

@BenKaufmann BenKaufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take a deeper look in the next couple of days.

@@ -328,6 +328,76 @@ class ClaspFacade : public ModelHandler {
*/
bool read();
//@}

/*!
* \name Userdefimed statistics functions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Userdefimed -> Userdefined

Copy link
Member

@pluehne pluehne Mar 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, even better, “User-defined,” UserDefinedStats, and so on.

UserdefinedStats();
const char* name() const;
size_t root() const;
/// check whether an Object (of a certain type) is already attached to a map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be consistent in the use of comment styles and please also document any exceptions and/or preconditions.

size_t root() const;
/// check whether an Object (of a certain type) is already attached to a map
/// if yes, target is set to the objects key
bool exists(size_t key, const std::string& name, size_t& target) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more natural interface would be to use a size_t-pointer as return value and then use nullptr to indicate the absence of name.

bool exists(size_t key, size_t index, size_t& target) const;
bool exists(size_t key, size_t index, Potassco::Statistics_t type, size_t& target) const;
/// attach a new Object to a Map or Array, or return the old one if if already exists
size_t get(size_t key, const std::string& name, Potassco::Statistics_t type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having get() to actually create an object feeels kinda wrong.

void set(size_t key, double v);

StatisticObject toStats();
private:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indentation

@@ -449,6 +519,8 @@ class ClaspFacade : public ModelHandler {
BuilderPtr builder_;
SummaryPtr accu_;
StatsPtr stats_; // statistics: only if requested
std::vector<UserdefinedStatsCallback> userStatsCbs_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a single vector storing pairs of function pointer and corresponding data.

return root_;
}

bool ClaspFacade::UserdefinedStats::exists(size_t key, const std::string& name, size_t& target) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way to much duplication!

}
auto v = connections_.find(key);
if (v == connections_.end()) { return false; }
for (const auto& i : v->second) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are sparse arrays really needed? The existing stats arrays all behave like normal c++ arrays, i.e. all elements in the range [0, size) are considered valid.

}
case Potassco::Statistics_t::Array: {
size_t size = vecs_[origin].size();
/// initialize empty fields with zeros ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above. I would already setup the arrays on construction.

size_t ClaspFacade::UserdefinedStats::get(size_t key, const std::string& name, Potassco::Statistics_t type) {
size_t target;
if (!exists(key,name,type,target)) {
switch(type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow clasp's indentation style (this also applies to other places)

@BenKaufmann
Copy link
Contributor

Closed. Superseded by #23

@MaxOstrowski MaxOstrowski deleted the wip branch April 13, 2018 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants