-
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
TableFactor Improvements #1556
TableFactor Improvements #1556
Conversation
@varunagrawal this PR shows changes in 15 files, just for adding a constructor? Are you sure it targets the right branch? |
I need to update the PR comments. I also moved a lot of common functionality up to the DisreteFactor class to keep it DRY. |
Hmm, yes, but TableFactor is a big change, and I'd rather review that in a separate PR if possible. |
Using TableFactor in elimination will be a separate PR for sure. |
@dellaert I've updated the PR comment to better reflect the changes. Please note that there is no functionality change in hybrid elimination in this PR, only variable name changes. |
OK. Any reason this is not targeting develop? |
7a3aa89
to
2b72f75
Compare
I chained PRs together since I wanted functionality from different PRs and was not getting review responses to unblock me. |
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.
Cool, looks right - feel free to merge after CI succeeds
TableFactor
which takes aDiscreteConditional
. The API for the TableFactor should now mimic that of theDecisionTreeFactor
.cardinalities_
anddiscreteKeys()
to theDiscreteFactor
class to reduce code duplication.