autonomio / talos

Hyperparameter Experiments with TensorFlow and Keras
https://autonom.io
MIT License
1.62k stars 268 forks source link

Added stopping after wall clock time #242

Closed JohanMollevik closed 5 years ago

JohanMollevik commented 5 years ago

Suggestion for how to add stopping after wall clock time as per https://github.com/autonomio/talos/issues/225

usage, pass max_iteration_start_time='YYYY-MM-DD HH:mm' to Scan

pep8speaks commented 5 years ago

Hello @JohanMollevik! Thanks for submitting the PR.

Line 155:38: E225 missing whitespace around operator

Line 14:1: W293 blank line contains whitespace Line 15:38: E711 comparison to None should be 'if cond is not None:' Line 16:17: E225 missing whitespace around operator Line 16:65: E231 missing whitespace after ',' Line 16:80: E501 line too long (82 > 79 characters) Line 25:42: E711 comparison to None should be 'if cond is not None:'

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 388


Changes Missing Coverage Covered Lines Changed/Added Lines %
talos/scan/scan_run.py 3 6 50.0%
<!-- Total: 4 7 57.14% -->
Totals Coverage Status
Change from base Build 386: 0.05%
Covered Lines: 1100
Relevant Lines: 1211

💛 - Coveralls
JohanMollevik commented 5 years ago

Something occured to me, would it be better to take a datetime object instead of a string as the scan parameter? What do you think, should I update the pull request?

mikkokotila commented 5 years ago

This is great. Thanks.

I'll merge it, but can you also add a test that specifically tests this (or edit one of the current tests accordingly).

Regarding the format, I think string is better as its clearer for a larger number of users.

JohanMollevik commented 5 years ago

Regarding testing, how do you propose to test it as it depends on wall clock time? The only reliable way I can figure out would be to start a long running scan and then add a stop time 1 min into the feature, but this will possibly be a rather slow test, how should I think about this?

mikkokotila commented 5 years ago

That's right. Now the test cycle is ~2 minutes so adding a whole minute (and actually it would have to be 2 minutes to avoid the case where the minute is just about to change if I understood correct) so that's off the table.

Related with this I have been thinking about far more robust testing that would just run locally that would take several hours and would take into consideration achieving given results in benchmark datasets.

But yes, it seems that for now it's better to leave it out from testing.