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

Better handling of empty Hoeffding trees #2964

Merged
merged 5 commits into from Jun 6, 2021

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Jun 1, 2021

In #2962, @xlindo pointed out some issues when trying to train a Hoeffding tree after it was created with an empty constructor. I went through the code and realized that there are some situations where this does not work correctly; therefore, I went through and refactored things such that the code in #2962 now works. In essence, the problem was that a HoeffdingTree<> stores a data::DatasetInfo and a few other parameters that track how many dimensions are in the data and what type those are, but the code does not provide clear facilities for how to modify those quantities.

Specifically, I made the following changes:

  • I added a bunch of documentation to the overloads of Train() to be clear about when the tree will and won't be reset.
  • I added a specific parameter resetTree to the overload of Train() that doesn't take a datasetInfo. This makes it possible to reset the internal state of a tree even without passing a DatasetInfo.
  • I refactored training and resetting into TrainInternal() and ResetTree(), which are meant to be called internally.
  • I added some new test cases to ensure that the original code works now.

@rcurtin rcurtin mentioned this pull request Jun 2, 2021
Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Looks great to me.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@zoq zoq merged commit 0259155 into mlpack:master Jun 6, 2021
This was referenced Oct 14, 2022
@rcurtin rcurtin mentioned this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants