Skip to content
This repository was archived by the owner on Jan 8, 2026. It is now read-only.

Conversation

@prakrutisingh24
Copy link
Contributor

No description provided.

from tornado.web import HTTPError
from sklearn.metrics import get_scorer
from sklearn.model_selection import cross_val_predict, cross_val_score
from sklearn.model_selection import cross_val_predict, cross_val_score
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line appears twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra line is unnecessary.

target = data[target_col]
train = data[[c for c in data if c != target_col]]
# cross validation
mod = cls.modelFunction()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required. The model is already present as cls.model, see line no: 116.

# cross validation
mod = cls.modelFunction()
CVscore = cross_val_score(mod, train, target)
CV = sum(CVscore)/len(CVscore)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Use CVscore.mean()
  2. Variable naming has to follow a specified style - do pip install flake8 and run the flake8 command against this file, i.e. flake8 mlhandler.py, and check the output.

@jaidevd
Copy link
Contributor

jaidevd commented May 13, 2021

@prakrutisingh24 In this PR, we are just computing the cross val score when the model is set up for the first time, and simply printing the CV score. What we need is:

  • When any scoring happens, especially if it is triggered by the user through an ?_action=train or ?_action=score - the result should be cross validated score
  • By default, cross validation should happen, but if the user wants, they should be able to turn it off through gramex.yaml, and get scores on the entire training dataset
  • sklearn's cross_val_scoire contains a parameter, cv, that can be set in many different ways. Check the docs for details. MLHandler users should be able to exercise all those options.

Thanks,

'cats': [],
'target_col': None,
'CV': True,
'CVargs': []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a single argument, cv, which can take any value, i.e in gramex.yaml, users should be able to write any of the following.

cv: false   # disable cross validation
cv: 5        # Use 5 folds
cv:
  cv: 8   # Use 8 folds
  n_jobs: -1  # with an optional other parameter.

from tornado.web import HTTPError
from sklearn.metrics import get_scorer
from sklearn.model_selection import cross_val_predict, cross_val_score
from sklearn.model_selection import cross_val_predict, cross_val_score
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra line is unnecessary.

# cross validation
print('yayyy we are here')
cls.CrossValidation(train,target)
print('should have printed')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the prints.

Copy link
Contributor

@jaidevd jaidevd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The training is happening in def _fit. Cross validation should also happen there.

from tornado.web import HTTPError
from sklearn.metrics import get_scorer
from sklearn.model_selection import cross_val_predict, cross_val_score
from ast import literal_eval
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be required.

'nums': [],
'cats': [],
'target_col': None,
'CV': True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it lowercase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to support three cases for the cv option:

  • If the user sets cv: false - then no cross validation happens
  • If the user sets cv: 4 (or some other integer) pass it straight to cross_val_score
  • The default should be cv: None, and in this case, the user should not have to write anything in gramex.yaml

target = data[target_col]
train = data[[c for c in data if c != target_col]]
# cross validation
cls.CrossValidation(train,target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it lowercase.

mclass = model_kwargs.get('class', False)
if mclass:
model = search_modelclass(mclass)(**model_kwargs.get('params', {}))
return model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not required.

if CV:
CVscore = cross_val_score(mod, X=train, y=target, **literal_eval(json.dumps(CV)))
CVavg = sum(CVscore)/len(CVscore)
print('Cross Validation Score : ',CVavg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CV should take place within the train method only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if cv:
    cvscore = cross_val_score(mod, X=train, y=target, cv=cv)
else:
   # Do the usual .fit

target = data[target_col]
train = data[[c for c in data if c != target_col]]
# cross validation
cls.cross_validation(train,target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants