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

Check if there is already old models that have been evaluated Fixes #540 #541

Merged
merged 1 commit into from
Dec 27, 2018

Conversation

avishekrk
Copy link
Contributor

When rerunning an experiment. This patch checks if the model_id has an entry in the evaluations table. If so, then it skips the evaluation saving time on longer model runs.

NOTE: If the type of evaluation parameters are different than the original model run the evaluation will still be skipped.

src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
Copy link
Member

@jesteria jesteria left a comment

Choose a reason for hiding this comment

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

And, BTW, the commit is messaged, "non-functioning check;" and, I'm not clear how this resolves #540…?

@thcrock
Copy link
Contributor

thcrock commented Dec 5, 2018

Per @jesteria 's comment, this doesn't appear to be complete. As it stands, besides a test, there are some changes that I think are important even to what is being sketched out:

  1. I do think we need to be sensitive to what evaluations are there. If we add a new evaluation metric, this should see it. We can probably make this check as coarse as: grab all of the metric/parameter combinations from the database for that model/date/as-of-date-frequency and compare with what is configured in the Evaluator. If not everything is there, re-do all of the evaluations.

  2. The bulk of the logic should be in the ModelEvaluator class, as that's where a lot of the related data lives already. It should expose something like a needs_evaluation method that takes in a model and a matrix_store

  3. This needs_evaluation should be called from the inner loop (where we actually call predict and evaluate).

@codecov-io
Copy link

codecov-io commented Dec 9, 2018

Codecov Report

Merging #541 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   82.63%   82.69%   +0.05%     
==========================================
  Files          82       82              
  Lines        4636     4651      +15     
==========================================
+ Hits         3831     3846      +15     
  Misses        805      805
Impacted Files Coverage Δ
src/triage/component/catwalk/evaluation.py 97.41% <100%> (+0.29%) ⬆️
src/triage/component/catwalk/model_testers.py 93.54% <100%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b89fce6...9a16646. Read the comment docs.

@thcrock
Copy link
Contributor

thcrock commented Dec 10, 2018

I added some test cases for a ModelEvaluator.needs_evaluations method to this branch that can serve as a guide to finishing this feature

src/tests/catwalk_tests/test_evaluation.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/evaluation.py Outdated Show resolved Hide resolved
src/triage/component/catwalk/model_testers.py Outdated Show resolved Hide resolved
- Add ModelEvaluator.needs_evaluation that checks whether or not any
configured evaluation metrics/parameters are missing in the database
- Call ModelEvaluator.needs_evaluation from ModelTester and skip both
prediction and evaluation if none is needed.
@thcrock thcrock merged commit 1e8b233 into master Dec 27, 2018
@thcrock thcrock deleted the check_for_old_models branch December 27, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants