[MRG] Precompute X_argsorted in AdaBoost

[glouppe]

This fixes #1667.

[ogrisel]

LGTM.

[amueller]

sweet, that was fast :)

[amueller]

Can we test somehow that it does the right thing?

[ogrisel]

BTW: if we used isinstance(base_estimator, BaseDecisionTree) instead of the inspect trick that would prevent to use bagging ensemble of trees as the base estimator for AdaBoost so I am ok with keeping the inspect trick.

[mblondel]

-1 on using inspect. It's a really ugly hack.

[glouppe]

What shall be used instead? Indeed, that trick also works for forests of randomized trees.

if isinstance(base_estimator, BaseDecisionTree) or isinstance(base_estimator, BaseForest) ?

(I have no strong opinion about that. I will be happy as soon it speeds things up for trees.)

[mblondel]

@glouppe The solution based on isinstance seems nicer. If you want to allow external estimators, you could also implement a fit_argsorted method and check its presence with if hasattr(base_estimator, "fit_argsorted").

[glouppe]

I changed for isinstance. Actually, forests do not have a X_argsorted fit parameter (I thought they were), hence that optimization cannot apply to them at the moment. I'll fix that later today or tomorrow.

[amueller]

-1 on isinstance. It relies on inheritance which we usually try to avoid.
Also -1 on adding another function. Why blow up the API like that?
@mblondel why do you think inspection is an ugly hack? We use it a lot in the basic api (for get_params for example).

[mblondel]

My gut feeling is that if we have to resort to something like that, something went wrong.
If inspecting the parameters is such a nice idea, why do we force unsupervised estimators to have a y parameter?
Regarding fit_argsorted, if you prefer, it could be considered an internal method and prefixed with a _.

[ogrisel]

I am ok for isinstance in the short term as the X_argsorted optim can currently only be implemented for DecisionTree and it's the least hackish / invasive method for now.

Alternatively I am also ok for _fit_argsorted (private) to avoid a combinatorial explosion of the public API. This optim is really specific for tree based models and I don't think it deserves extending the supported public API.

The later option would require refactoring the BaseDecisiontTree class a bit though.

[amueller]

Ok, I guess the private method would be the nicest solution as it allows usual ducktyping.
@mblondel I think testing whether fit takes a y argument might actually be nice ;) The problem with that kind of inspection is that we test the argument names which is probably not that stable, i.e. if if is called Y somewhere it would suddenly break :-/

[glouppe]

So basically, can this be merged as this or shall I add now _fit_argsorted in BaseDecisionTree and in BaseForest? (This requires some work, but I can do it sometimes this week.)

[amueller]

feel free to merge

[ogrisel]

@glouppe Please open a new issue to implement the _fit_argsorted solution to make it AdaBoost work both with simple trees and ensemble of trees as base estimators.

[GaelVaroquaux]

BTW: if we used isinstance(base_estimator, BaseDecisionTree) instead of the
inspect trick that would prevent to use bagging ensemble of trees as the base
estimator for AdaBoost so I am ok with keeping the inspect trick.

:). Relying on inheritence is troublesome for software design.