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

added printing status and tests for it #1168

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mnoukhov
Copy link

Printing extension had default print_status=True without the ability to change it, I'm just adding it as a an argument.

Also added tests to a new test_printing.py, I found there was a printing test in test_progressbar but that felt more like a integration test than a unit test for something so simple as the print_status argument.

@rizar
Copy link
Contributor

rizar commented Jan 2, 2017

Sorry for the late reply and thanks for the PR! I am a bit hesitant to merge it though. There is so many ways in which Printing could be customized, and it would never fit everybody's needs. This is why we have been keeping it primitive as it is so far.

@mnoukhov
Copy link
Author

mnoukhov commented Jan 3, 2017

That makes sense, but the code already has just an unused variable print_status that is just never set. This change makes it usable and adds the ability to make normal runs (that don't need the status, which I imagine would be the majority) much cleaner.

@rizar
Copy link
Contributor

rizar commented Jan 9, 2017

I think most people prefer to print status, because without printing it one doesn't even know how many iterations of training have been done.

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.

2 participants