-
Notifications
You must be signed in to change notification settings - Fork 768
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
Efficient Discrete Elimination #1590
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.
Again a PR that seems to be mixing a key new implementation of probabilities
and some unrelated TableFactor changes? Split if possible, and I'd love to see
- an explanation in the code as to how the new implementation works and how it assigns a probability for all assignments
- some unit tests with pruned trees? Do they exist already?
} | ||
|
||
// Go through the tree | ||
this->apply([&](const Assignment<Key> a, double p) { |
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.
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.
I'd prefer if you don't use the lambda in place but create op
using an auto and properly document what it does.
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
for (auto&& k : diff) { | ||
nrAssignments *= cardinalities_.at(k); | ||
} | ||
probs.insert(probs.end(), nrAssignments, p); |
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.
What is happening here? probs is a vector, right?
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.
Yes. This is adding the value p
to probs
, nrAssignments
times.
This is because I also didn't want to waste CI resources and have to wait potentially another couple of weeks for a review, hence the kitchen sink approach. |
@dellaert updated as per review. I already have a whole unit test dedicated to pruning in |
Updated the discrete elimination process to be ultra performant.
Current timing results:
We have successfully obtained sub-millisecond performance on this test.