-
Notifications
You must be signed in to change notification settings - Fork 50
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
io: allow setting progress=False in solver to disable stdout even for larger models #375
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you @maurerle
A few minor things left on types and docstrings.
b1e17cf
to
303585a
Compare
303585a
to
2cfdba1
Compare
79de0e5
to
4e0b2b3
Compare
7a21b43
to
4dee914
Compare
Looks good from my side now, @lkstrp can you take another look please? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
Two last things:
- Release note is missing
- We can deprecate
log
instead of removing it. So accept bothlog
andprogress
, and throw aDeprecationWarning
whenlog
is passed. There is a decorator for this somewhere, I can add it if you don't mind. It may be unnecessary, but I think it's better practice.
If you add the remarks yourself, that would be great! From my point of view, the deprecation is not really necessary as there did not exist a public api to set log before anyway. |
This was unnecessary when solving a lot of problems programatically.
The
log
param of the solve function does have three states:None
: log tqdm to stdout if model is > 10k constraintsTrue
: always creates tqdm for modelFalse
: never creates tqdm for modelFor better usage, the log param is renamed to
progress
on all occurrences