Implements chooseUnorderedCategoricalSplit

[feynmanliang]

TODOs:

• Switch to 1 vs all when featureArity is too large (heuristic here?)
• Efficiently propose/iterate over proposed subset splits; we currently add and remove all ImpurityAggregatorSingles for each category from the left and right ImpurityStatss when we should just be making a single add/remove from left/right (ditto for leftCount and rightCount)

[feynmanliang]

@jkbradley Rewrote this to consider all subsets for splitting unordered categorical, do you mind reviewing? Also, see TODOs in PR heading; can those wait for another PR or do you want them in this one?

[jkbradley]

Thanks! I'll take a look.

Switch to 1 vs all when featureArity is too large (heuristic here?)

Definitely should not do it here. I'm not sure if this will be better than the currently heuristic of imposing an ordering.

Efficiently propose/iterate over proposed subset splits; we currently add and remove all ImpurityAggregatorSingles for each category from the left and right ImpurityStatss when we should just be making a single add/remove from left/right (ditto for leftCount and rightCount)

I agree we should wait on this.

[jkbradley]

Remove "possibly"

[feynmanliang]

OK

[jkbradley]

That's all for now. My comments are just cleanups, but I think it can be simplified a bit. Thanks!

[feynmanliang]

@jkbradley Updated

[jkbradley]

This modifies fullImpurityAgg. It should create a copy first (or share 1 copy for the whole loop and overwrite on each iteration).

[feynmanliang]

OK

[jkbradley]

@feynmanliang Thank you for the updates! Just a couple new comments.

[feynmanliang]

@jkbradley Updated

[jkbradley]

Thanks for updating! This LGTM. I'll merge it.