Working on setup and train functionality #327
Conversation
reed0824
temporarily deployed
to
Integrate Pull Request
GitHub Actions
Inactive
reed0824
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
There was a problem hiding this comment.
Hi @reed0824, here are some comments on your latest draft. In addition to the itemized recommendations, I suggest that future PRs come from a branch of your forked repo, rather than your main branch. To create a new branch called branchname in the command line, you can use git checkout -b branchname.
| def __init__( | ||
| self, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
Probably can remove this __init__ method. setup will take care of all the initializations.
| # | ||
| def summary(self, **kwargs): | ||
|
|
||
| model = xgb.train( |
There was a problem hiding this comment.
Remember to use self.model here, in addition to any other places where you'd like to set an attribute (variable accessible by all the different methods in the XGB class).
| def setup( | ||
| self, | ||
| 'max_depth':6, | ||
| 'min_child_weight': 1, | ||
| 'eta':.1, | ||
| 'subsample': 0.7, | ||
| 'colsample_bytree': 0.7, | ||
| 'objective': 'binary:logistic', | ||
| 'eval_metric': 'auc', | ||
| **kwargs, |
There was a problem hiding this comment.
Nice, I think this is a good way to initialize the model parameters.
| self.model.compile( | ||
| params =self.meta["params"], | ||
| train = self.dtrain, | ||
| test = self.dtest, | ||
| ) |
There was a problem hiding this comment.
I don't see compile in the XGBoost notebook. Should it be included here or at all?
There was a problem hiding this comment.
Possibly not; I copied the same format as nn.py here and model.compile does not yet exist for xgb
|
|
||
| def train(self,X_train, **kwargs): | ||
| dtrain = xgb.DMatrix(X_train, label=y_train) | ||
|
|
There was a problem hiding this comment.
This is a good place to add something like self.model = xgb.train(...).
|
|
||
| return dtrain | ||
|
|
||
| def test(self, X_test, **kwargs); |
There was a problem hiding this comment.
I'm not sure if we need a separate test method. Perhaps it's best for now to define dtest like you do for dtrain within the train method, then pass it in to the call to xgb.train(...), e.g. using evals=[(dtest, "Test")]. You'd need to add Xtest as another argument to the train method.
reed0824
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
reed0824
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
reed0824
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
reed0824
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
I've updated the setup() function, and added train/call/summary/test to the class. Any comments are welcome if you have time, Brian.
I have temporarily removed plotting functionality