-
Notifications
You must be signed in to change notification settings - Fork 766
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
Fix hybrid bug, double performance #1857
Conversation
Note: “doubling performance” refers to testHybridEstimation. Could be twice as fast because now the discrete factors are properly taken into account when pruning. |
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.
Some questions, especially about a copy.
gtsam/hybrid/HybridBayesNet.cpp
Outdated
DecisionTreeFactor prunedDiscreteProbs = | ||
this->pruneDiscreteConditionals(maxNrLeaves); | ||
copy.pruneDiscreteConditionals(maxNrLeaves); |
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.
Why a copy if everything is functional?
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.
You're right, it was left-over ! I'll fix
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.
Actually, I messed up the discrete part. Did you ever profile the pruning as it is? It's an exponential operation - that alone might be responsible for slowness in some experiments.
I'll fix but we have to chat about this more I think.
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 haven't. I would like to see your perf-profiling setup and make it standard across projects. :) Seems to be very useful.
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'm on a mac. I just build the binary and use Instruments CPU profiler.
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.
OK, I fixed it.
- You probably had pruning tests in your project, but I added some here so I could refactor HGC::prune based on ADT::max.
- You might want to check out his branch and try whether your experiments still work before we merge?
@varunagrawal please merge this after you established your experiments work? |
This looks great so merging! |
I could not really separate the functional pruning PR from this one. Here's everything in this PR:
Fixing the bug
discretePosterior
HBN::evaluate
andHGFG::probPrime
that take VectorValues todiscretePosterior
: these are very powerful.Simplified API
HBN::logProbability
: errorTree is all we need and this one has confusing semanticsHGC::logProbability
that takes continuous values: very problematic name and only used in one place: again, errorTree is all we need.Functional pruning
Update Oct 1:
HGC::prune
to useADT::max
HybridBayesNet::prune
completelyLooking at pruned trees for testHybridEstimation is very informative: it seems that the ordering of the keys is working against us in terms of pruning: we have large duplicate trees:
step 2:
step 5:
...
step 14: