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 progress report to console when running sourmash compute with --singleton #84

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

Conversation

lgautier
Copy link
Contributor

@lgautier lgautier commented Jan 3, 2017

Print a progress report when computing signatures.

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@codecov-io
Copy link

codecov-io commented Jan 3, 2017

Current coverage is 76.69% (diff: 100%)

Merging #84 into master will increase coverage by 0.10%

@@             master        #84   diff @@
==========================================
  Files            17         17          
  Lines          2273       2283    +10   
  Methods          46         46          
  Messages          0          0          
  Branches         84         84          
==========================================
+ Hits           1741       1751    +10   
  Misses          507        507          
  Partials         25         25          

Powered by Codecov. Last update 86adefe...3fb7871

@@ -294,13 +297,26 @@ def save_siglist(siglist, output_fp, filename=None):
if args.singleton:
siglist = []
for n, record in enumerate(screed.open(filename)):
if n % args.progress_stepsize == 0:
if sys.version_info[0] < 3:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think we should avoid adding too many of these checks to the code. Is it too bad to just not flush the output?

And not for this PR, but we should definitely start using the logging module for this kind of output, instead of printing stuff. Also makes it easier to filter what output we want (INFO for printing progress, for example?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think we should avoid adding too many of these checks to the code. Is it too bad to just not flush the output?

It is a progress report. Not flushing the output kind of defeats the purpose.

And not for this PR, but we should definitely start using the logging module for this kind of output, instead of printing stuff. Also makes it easier to filter what output we want (INFO for printing progress, for example?)

Big yes about using logging (I am trying to match the current code base and not have changes and new features are the same time to the extent possible - I am happy to go both if not creating too much trouble). Note: Flushing is implicitly done by logging with each call, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not call sys.stderr.flush() in both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would work... although it doesn't make the implicit point that Python 2 should eventually go ;-)

@ctb
Copy link
Contributor

ctb commented Jan 3, 2017 via email

@ctb
Copy link
Contributor

ctb commented Jan 3, 2017

If interested in logging, a super lightweight way to go about it is here:

https://github.com/dib-lab/syrah/blob/master/syrah#L25

Also see discussions/code here and here - the latter is probably better suited to sourmash, which is much lighter-weight than khmer.

(See #89)

@lgautier
Copy link
Contributor Author

lgautier commented Jan 3, 2017

@luizirber : I only realize now that I missed the one little element (easy to miss) that might have caused the question about flush. I am using end="\r" in the print statement. This has the benefit of making the a progress prints stay on the same line in the terminal, but it is also requiring a flush to be sure that the output on the screen is updated (the character for a new line triggering a flush of stderr).

Python 3, you can try the following standalone snippet:

import time
for x in range(1, 101):
    print('iteration %i' % x, end="\r", flush=True)
    time.sleep(.5)

@ctb
Copy link
Contributor

ctb commented Jan 4, 2017 via email

@ctb
Copy link
Contributor

ctb commented Jan 21, 2017

This needs to be updated for latest master. @lgautier I would have to create a new branch & PR to do so - is that OK?

@lgautier
Copy link
Contributor Author

@ctb - sure. no pb. go ahead.

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.

None yet

5 participants