-
Notifications
You must be signed in to change notification settings - Fork 17
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
ML XGBoost Classification #484
Conversation
…other processes. Default to numerical index instead of string. (#478)
* `filter_spatial`: Clarified that a masking get applied for the given geometries. #469 * `filter_bbox`: Clarified that the bounding box is reprojected to the CRS of the spatial data cube dimensions if required. --------- Co-authored-by: Stefaan Lippens <[email protected]>
"type": "object", | ||
"subtype": "ml-model", | ||
"title": "Machine Learning Model", | ||
"description": "A machine learning model, accompanied with STAC metadata that implements the the STAC ml-model extension." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accompanied with STAC metadata that implements the the STAC ml-model extension
What does this practically mean here in this context of defining a JSON schema? Isn't that more a concern of a process like save_ml_model
that actually "exports" the model to a more concrete form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accompanied with STAC metadata that implements the the STAC ml-model extension
What does this practically mean here in this context of defining a JSON schema? Isn't that more a concern of a process like
save_ml_model
that actually "exports" the model to a more concrete form?
This was to remove the error due to the ml model being returned. I created a branch from the draft and not the ml branch.
proposals/ml_fit_class_xgboost.json
Outdated
} | ||
}, | ||
{ | ||
"name": "seed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of parameters are listed here. Are these based on a particular xgboost implementation? And are we sure that they are translatable to other implementation? Otherwise I think it would make sense to drop a couple from this initial spec, and leave some wiggle room for backend implementers
I remember we had this same issue with the random forest process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had checked python and R libraries and these params can be specified : https://xgboost.readthedocs.io/en/stable/parameter.html
I can mabye remove 3 : early_stopping_rounds, nfolds and nrounds to be dealt with internally. I set default values for most params though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PondiB I think it would make sense to make PRs against the ml branch because otherwise all changes from the ML branch will also appear in this PR. This leads to confusion as you can see with @soxofaan's comments. Please rebase your changes against the ML branch if necessary and set the base branch of the PR to ml.
hmm this PR now has "27 files changed", most of the changes are irrelevant to the original issue (ML XGBoost) |
Lol, Never noticed it in the evening. I did run npm test locally no idea how it led to this. I will rebase the commit and push again. |
b83aaad
to
e98dd7f
Compare
Moved to #487 |
Specification for ML XGBoost for Classification.
Depends on #441