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

Changes in gtsam to support hybrid branches #1055

Merged
merged 16 commits into from
Jan 23, 2022
Merged

Conversation

dellaert
Copy link
Member

This PR collect a number of commits to files made in the hybrid development branches.
I made the commits below descriptive.
@varunagrawal we can re-target the PRs or just wait until this is merged.

@dellaert dellaert self-assigned this Jan 22, 2022
@dellaert dellaert added the quick-review Quick and easy PR to review label Jan 22, 2022
@@ -340,4 +340,11 @@ namespace gtsam {
return f.apply(g, op);
}

/// unzip a DecisionTree if its leaves are `std::pair`
template<typename L, typename T1, typename T2>
std::pair<DecisionTree<L, T1>, DecisionTree<L, T2> > unzip(const DecisionTree<L, std::pair<T1, T2> > &input) {
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, @ProfFan , this should probably have been a static method and be called Unzip. Or a method, and then it would not take an argument, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, just looking for consensus, I think I would change it to static.

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 see now it is neither: it is a free function :-/ Which is correctly named, so never mind! Will re-format is all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to reopen CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll just do the required one and merge

@dellaert
Copy link
Member Author

Killed CI while deciding about Unzip

@dellaert dellaert requested a review from ProfFan January 22, 2022 18:47
# Conflicts:
#	gtsam/discrete/DecisionTreeFactor.cpp
@dellaert
Copy link
Member Author

I think this is ready to merge if I can get an approving review from @ProfFan or @varunagrawal

@varunagrawal
Copy link
Collaborator

Sorry I was out the entire afternoon. I'll review in the next 30 minutes.

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! Merge whenever.

@dellaert dellaert marked this pull request as draft January 22, 2022 23:11
@dellaert dellaert marked this pull request as ready for review January 22, 2022 23:11
@dellaert
Copy link
Member Author

I can't merge because there is some CI waiting to be reported. Any ideas?

@ProfFan ProfFan closed this Jan 22, 2022
@ProfFan ProfFan reopened this Jan 22, 2022
@ProfFan
Copy link
Collaborator

ProfFan commented Jan 22, 2022

I reopened the PR, now it should start to build. @dellaert

@ProfFan
Copy link
Collaborator

ProfFan commented Jan 23, 2022

@dellaert CI is ready :)

@dellaert dellaert merged commit 9d71c90 into develop Jan 23, 2022
@dellaert dellaert deleted the feature/hybrid_base branch January 23, 2022 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick-review Quick and easy PR to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants