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 DecisionTreeFactor division #1962

Merged
merged 10 commits into from
Jan 6, 2025
Merged

Fix DecisionTreeFactor division #1962

merged 10 commits into from
Jan 6, 2025

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Jan 3, 2025

Added a unit test that shows the number of keys is incorrect on division, and subsequently fixed it improves the understanding of discrete factor division.

Also added a Signature based constructor and updated the tests to use it.

@varunagrawal varunagrawal requested a review from dellaert January 3, 2025 18:30
@varunagrawal varunagrawal self-assigned this Jan 3, 2025
@dellaert
Copy link
Member

dellaert commented Jan 3, 2025

Hmmm, a signature-based constructor only makes sense to me for conditionals (or distributions). I think this mixes up the semantics

@varunagrawal varunagrawal requested a review from dellaert January 3, 2025 18:43
@dellaert
Copy link
Member

dellaert commented Jan 3, 2025

I see why you did this, when looking at the tests, but there is a reason I left it in the tests: it is because signatures are a conditional thing. I think a better way is to create conditionals in the tests, which are derived from DTF anyway, so you can use them as factors in the tests

@varunagrawal
Copy link
Collaborator Author

I see why you did this, when looking at the tests, but there is a reason I left it in the tests: it is because signatures are a conditional thing. I think a better way is to create conditionals in the tests, which are derived from DTF anyway, so you can use them as factors in the tests

It doesn't really matter to be honest. I thought it was better to have Signature constructor since the create method was doing that indirectly, but I didn't have the full picture.

@dellaert
Copy link
Member

dellaert commented Jan 4, 2025

CI fails now, maybe create was not re-instated?
Actually, I think my suggested change, creating conditionals to create the factors made sense, no?

@varunagrawal
Copy link
Collaborator Author

From the diff, it looks like create was added back. I'll look into this some more.

@varunagrawal
Copy link
Collaborator Author

On more thinking about this, I believe the mathematical structure should include all keys since we would have functional division $(\frac{f}{g})(x, y) = f(x, y) / g(x)$.

I'll update the unit test to showcase this and update the docstring.

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Cool!

@varunagrawal varunagrawal merged commit ffd04fd into develop Jan 6, 2025
33 checks passed
@varunagrawal varunagrawal deleted the fix-dtf-division branch January 6, 2025 15:26
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