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

Fix hybrid bug, double performance #1857

Merged
merged 22 commits into from
Oct 6, 2024
Merged

Fix hybrid bug, double performance #1857

merged 22 commits into from
Oct 6, 2024

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Oct 1, 2024

I could not really separate the functional pruning PR from this one. Here's everything in this PR:

Fixing the bug

  • BIG: fix bug in HybridConditional::errorTree and HGFG::errorTree
  • made errorTree much faster by calling polymorphic errorTree and using AlgebraicDecisionTree operators

discretePosterior

  • renamed both HBN::evaluate and HGFG::probPrime that take VectorValues to discretePosterior: these are very powerful.

Simplified API

  • removed HBN::logProbability: errorTree is all we need and this one has confusing semantics
  • removed HGC::logProbability that takes continuous values: very problematic name and only used in one place: again, errorTree is all we need.

Functional pruning

  • pruning is now functional
  • got rid of duplicate (unused) prunerFunc
  • added a write-access conditional to allow iSAM versions to be still imperative

Update Oct 1:

  • Reworked HGC::prune to use ADT::max
  • Rewrote HybridBayesNet::prune completely

Looking 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:
image

step 5:
image

...

step 14:
image

@dellaert
Copy link
Member Author

dellaert commented Oct 1, 2024

Note: “doubling performance” refers to testHybridEstimation. Could be twice as fast because now the discrete factors are properly taken into account when pruning.

Copy link
Collaborator

@varunagrawal varunagrawal left a 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.

DecisionTreeFactor prunedDiscreteProbs =
this->pruneDiscreteConditionals(maxNrLeaves);
copy.pruneDiscreteConditionals(maxNrLeaves);
Copy link
Collaborator

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?

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I fixed it.

@varunagrawal :

  • 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?

gtsam/hybrid/HybridBayesNet.h Outdated Show resolved Hide resolved
@dellaert
Copy link
Member Author

dellaert commented Oct 3, 2024

@varunagrawal please merge this after you established your experiments work?

@varunagrawal
Copy link
Collaborator

This looks great so merging!

@varunagrawal varunagrawal merged commit b89e9c9 into develop Oct 6, 2024
33 checks passed
@varunagrawal varunagrawal deleted the feature/posteriors branch October 6, 2024 17:05
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.

2 participants