-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support distribution constructor args and enable DirichletCategorical. #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks very nice!
cxx/distributions/adapter.hh
Outdated
@@ -58,5 +58,9 @@ class DistributionAdapter : public Distribution<std::string> { | |||
|
|||
void transition_hyperparameters() { d->transition_hyperparameters(); } | |||
|
|||
DistributionAdapter* prior() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to delete DistributionAdapter if we aren't using it in your design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (args_str.empty()) { | ||
return DistributionSpec{dist}; | ||
} else { | ||
assert(args_str[0] == '('); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a lot cleaner if it used regular expressions (using Boost.Regex say). For example, that way you could parse out the distribution name and then look it up in a map from names to DistributionEnum values and not have to do it through a chain of if's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- I parsed out the distribution name just using substr
and find
without regex (if you have ideas as to how to clean it up further using regex let me know, my regex-fu is not very good).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested the following code, but it should be something like
#include <boost/regex.hpp>
boost::regex re{"^(\w+)(?:(((\w+)\s*=\s*(\w+),?)*))?$"};
boost:smatch match;
if (boost::regex_search(dist_str, match, re)) {
string dist_name = match[1];
// First keyword will be in match[2][0] and its value in match[2][1] etc.
}
cxx/util_distribution_variant.cc
Outdated
case DistributionEnum::bigram: | ||
return value_str; | ||
default: | ||
assert(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, printing an error message before aborting would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cxx/relation.hh
Outdated
// Relation spec. | ||
T_relation trel; | ||
// Distribution prior over the relation's codomain. | ||
const DistributionType* cluster_prior; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than storing this as a DistributionType*, you could store the DistributionSpec and just use cluster_prior_from_spec to generate the new distributions when needed. That way you wouldn't need to add the prior() method to all the distributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that! In an earlier iteration I was storing the whole distribution_args string in DistributionSpec, and I wanted to avoid re-parsing the string every time a new prior instance was needed, but now that DistributionSpec contains an argname/argval map I agree this is better.
class Relation; | ||
|
||
// Set of all distribution sample types. | ||
using ObservationVariant = std::variant<double, int, std::string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a
// TODO(thomaswc): Change BetaBernoulli to use bool's instead of double's.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, in the BetaBernoulli class. This was on my list of things to get to also.
0e914d9
to
9ae3928
Compare
if (args_str.empty()) { | ||
return DistributionSpec{dist}; | ||
} else { | ||
assert(args_str[0] == '('); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested the following code, but it should be something like
#include <boost/regex.hpp>
boost::regex re{"^(\w+)(?:(((\w+)\s*=\s*(\w+),?)*))?$"};
boost:smatch match;
if (boost::regex_search(dist_str, match, re)) {
string dist_name = match[1];
// First keyword will be in match[2][0] and its value in match[2][1] etc.
}
I haven't managed to get this working, so I'm going to leave it as-is for now and maybe come back to it (feel free to send a PR if you get around to it too). |
This also addresses @ThomasColthurst 's remaining comments from #33 and #40 .