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.

Sign up for GitHub

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

Jump to bottom

DecisionTree refactor #1000

Merged
merged 27 commits into from
Jan 3, 2022
Merged

DecisionTree refactor #1000

merged 27 commits into from
Jan 3, 2022

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Jan 2, 2022

For hybrid elimination, we need DecisionTree to be more powerful.

  • Previously there was a restriction to values that could be printed using <<. This PR adds label formatters (done previously in a branch by Varun) and value formatters (by me, here).
  • In addition, equals no longer takes a tolerance, but a compare function.
  • DT is no longer Testable, but AlgebraicDecisionTree is.

varunagrawal and others added 19 commits January 2, 2022 16:34
@varunagrawal @dellaert
fix decision tree equality and move default constructor to public
94f2135
@varunagrawal @dellaert
add formatting capabilities to DecisionTree
ddaf960
@varunagrawal @dellaert
revert incorrect change
8b5d93a
@varunagrawal @dellaert
new DecisionTree constructor and methods that takes an op to convert …
bb6e489 
…from one type to another

# Conflicts:
#	gtsam/hybrid/DCMixtureFactor.h
@varunagrawal @dellaert
minor format
315b10b
@varunagrawal @dellaert
added SFINAE methods for Leaf node equality checks
28071ed
@varunagrawal @dellaert
replace dot with DOT to prevent collision with vector dot product
f1dedca
@varunagrawal @dellaert
minor fix
1c76de4
@varunagrawal @dellaert
undo change to test
573d0d1
@varunagrawal @dellaert
formatter passed as reference and added a default formatter method
ed83908
@varunagrawal @dellaert
undo dot changes
9982057
@varunagrawal @dellaert
add comparator as argument to equals method
b24da83 
# Conflicts:
#	gtsam/hybrid/DCMixtureFactor.h
@varunagrawal @dellaert
address more review comments
26c48a8 
# Conflicts:
#	gtsam/hybrid/DCGaussianMixtureFactor.h
#	gtsam/hybrid/DCMixtureFactor.h
@varunagrawal @dellaert
rename comparator to compare and capture tol in the function lambda.
731cff7 
# Conflicts:
#	gtsam/hybrid/DCMixtureFactor.h
@dellaert
Removed copy/paste convert
7f3f332
@dellaert
Define empty and check for it in apply variants
78f8cc9
@dellaert
Refactor print, equals, convert
db3cb4d
@dellaert
Fixed dot to have right arguments
5c4038c
@dellaert
Renamed protected method convert -> convertFrom
6c23fd1
@dellaert dellaert self-assigned this Jan 2, 2022
@dellaert dellaert requested a review from ProfFan January 2, 2022 23:13
@dellaert dellaert removed the request for review from ProfFan January 3, 2022 03:31
varunagrawal
varunagrawal approved these changes Jan 3, 2022
View reviewed changes
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.

LGTM, though hold up on merging since I'll add docstrings and other stuff addressing my comments in a separate PR targeting this PR.

gtsam/discrete/AlgebraicDecisionTree.h Show resolved Hide resolved
gtsam/discrete/DecisionTree-inl.h Show resolved Hide resolved
gtsam/discrete/tests/testDecisionTree.cpp Outdated Show resolved Hide resolved
gtsam/discrete/AlgebraicDecisionTree.h Show resolved Hide resolved
@varunagrawal varunagrawal mentioned this pull request Jan 3, 2022
Merged
@dellaert dellaert changed the title fix decision tree equality and move default constructor to public DecisionTree refactor Jan 3, 2022
Base automatically changed from feature/deprecate_in_v42 to develop January 3, 2022 19:16
@dellaert dellaert merged commit 1574227 into develop Jan 3, 2022
@dellaert dellaert deleted the feature/decison_tree branch January 3, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varunagrawal varunagrawal varunagrawal approved these changes

Assignees

@dellaert dellaert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dellaert @varunagrawal