Skip to content

Warning if fitted sklearn model being used #989

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.

Sign up for GitHub

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

Merged
PGijsbers merged 9 commits into from
Nov 3, 2020
Merged

Warning if fitted sklearn model being used #989

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d224845
sklearn model fit check
Neeratyoy Oct 28, 2020
409c6c0
Merge branch 'develop' into fix_968
Neeratyoy Oct 30, 2020
01a7c36
Reordering warning
Neeratyoy Oct 30, 2020
d08ad83
Update openml/extensions/sklearn/extension.py
mfeurer Nov 2, 2020
c72baee
Adding function to ext for checking model fit
Neeratyoy Nov 2, 2020
274506b
resolving merge conflicts
Neeratyoy Nov 2, 2020
acb8d66
Removing junk file
Neeratyoy Nov 2, 2020
3c3b3d8
Fixing sklearn version compatibility issue
Neeratyoy Nov 2, 2020
1e90133
Merge branch 'develop' into fix_968
Neeratyoy Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
sklearn model fit check
  • Loading branch information
@Neeratyoy
Neeratyoy committed Oct 28, 2020
commit d224845a327e9e5e754f8e3b7a9c2ffc5c7fbe8f
16 changes: 16 additions & 0 deletions openml/extensions/sklearn/extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1652,6 +1652,22 @@ def _prediction_to_probabilities(y: np.ndarray, model_classes: List[Any]) -> pd.

user_defined_measures = OrderedDict() # type: 'OrderedDict[str, float]'

try:
# check if model is fitted
# 'predict' internally calls sklearn.utils.validation.check_is_fitted for every
# model-specific attribute it excepts, thus offering a more robust check than
Comment thread
mfeurer marked this conversation as resolved.
Outdated
# a generic simplified call of check_is_fitted(model_copy)
from sklearn.exceptions import NotFittedError

model_copy.predict(X_train)
warnings.warn(
"The model is already fitted!"
" This might cause inconsistency in comparison of results."
)
except NotFittedError:
# model is not fitted, as is required
pass
Copy link
Copy Markdown
Collaborator

@PGijsbers PGijsbers Oct 30, 2020

Choose a reason for hiding this comment

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

I thought in the Python call we discussed that perhaps we would check this at the first call to run_model_on_task?
In either case I would extract this to a separate method _raise_warning_if_fitted to make sure the functions don't get too big (they already are).

Copy link
Copy Markdown
Contributor Author

@Neeratyoy Neeratyoy Oct 30, 2020

Choose a reason for hiding this comment

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

Alright, will make it into a function and push.

As for its placement, I reconsidered it given that irrespective of what is called, run_model_on_task or run_flow_on_task, this function is what the call is reduced to. Hence went ahead with this placement for this snippet of code.

Copy link
Copy Markdown
Collaborator

@PGijsbers PGijsbers Oct 31, 2020

Choose a reason for hiding this comment

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

run_model_on_task actually calls run_flow_on_task. That said, then we would need to add a function to the extension interface that will indicate if a model is already fit, otherwise we can't check it in a general way.

Copy link
Copy Markdown
Collaborator

@PGijsbers PGijsbers Oct 31, 2020

Choose a reason for hiding this comment

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

@mfeurer do you think this is something we should want? or do we just leave it to the extension devs to implement a warning if they see it fit?

Copy link
Copy Markdown
Collaborator

@mfeurer mfeurer Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I like the idea of having this function as callback that can be implemented by the extension deves. And yes, I expected this function to be called from the run_model_on_task function.


try:
# for measuring runtime. Only available since Python 3.3
modelfit_start_cputime = time.process_time()
Expand Down