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

Finish XGBoost implementation. #65

Closed
3 tasks done
ablaom opened this issue Feb 5, 2019 · 2 comments
Closed
3 tasks done

Finish XGBoost implementation. #65

ablaom opened this issue Feb 5, 2019 · 2 comments
Labels
invalid This doesn't seem right

Comments

@ablaom
Copy link
Member

ablaom commented Feb 5, 2019

Yiannes has done a great job with the code at src/interfaces/XGBoost.jl but it is does not meet the model spec yet. The models may need to be split further according to "objective" function as Regressor/Classifier Deterministic/Probabilistic etc. And the classifiers need to be integrated with CategoricalArrays, preserving input levels, etc

I will try to have a look at this myself soon.

Added: It is natural to break the XGBoost model into three separate models, depending on the value of the original XGBoost parameter objective:

  • XGBoostRegressor <: Deterministic{Any} - for reg:linear, reg:gamma, reg:tweedie(target_scitype = Continuous). MLJ objective default: objective=:linear.

  • XGBoostCount <: Deterministic{Any} - for count:poisson (target_scitype=Count). MLJ objective hyperparameter has :poisson as only allowed parameter value.

  • XGBoostClassifier <: Probabilistic{Any} - for binary:logistic, multi:softprob (target_scitype = Union{Multiclass,FiniteOrderedFactor}). MLJ objective parameter has :automatic as only allowed value.

I don't think we should implement any of the other XGBoost objective options at this time. In particular, note that reg:logistic and multi:softmax are redundant. To get these one can use the probabilistic versions and call predict_mode instead of predict. Maybe the doc string can mention this. (We do not need to implement predict_mode because there is a fall-back in MLJBase.)

Notes:

  • Please, let's implement and test these one at a time and organise the code the same way (ie don't interweave code for the three models, this makes it harder to review.)

  • We should drop the num_class hyperparameter altogether, as we are inferring this from nlevels(y). This will avoid a lot of dancing around in clean! (The original XGBoost needs this parameter, because it has no way to know the complete pool of target values.)

  • Since, XGBoostClassifier is probabilistic, it will predict vectors of distributions of type MLJBase.UnivariateNominal. As discussed in the guide, we will need to decode the target using decoder=CategoricalDecoder(y, Int). I suggest bundling the the decoder with the fitresult to make it available to predict, and to reconstruct the labels (in the correct order) using inverse_transform(decoder, eachindex(levels(decoder)).

  • To reduce code redundancy, we may want to define a macro for the model struct declarations and keyword constructors. This can be done later, but with this in mind, we should keep the hyperaparameter list the same across the threee models, even ones that don't apply. We can use clean! to make relevant warnings.

@ablaom ablaom changed the title Finish [XGBoost.jl](https://github.com/dmlc/XGBoost.jl) implementation. Finish XGBoost implementation. Feb 5, 2019
@ablaom ablaom added the invalid This doesn't seem right label Feb 5, 2019
@ysimillides
Copy link
Collaborator

JuliaAI/MLJModels.jl#10

@ablaom
Copy link
Member Author

ablaom commented Apr 8, 2019

resolved: JuliaAI/MLJModels.jl#10 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants