search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
713 stars 38 forks source link

[REVIEW]: `tqdm`: A Fast, Extensible Progress Meter for Python and CLI #1277

Closed whedon closed 4 years ago

whedon commented 5 years ago

Submitting author: @casperdcl (da Costa-Luis, Casper O.) Repository: https://github.com/tqdm/tqdm Version: v4.32.1 Editor: @cMadan Reviewer: @GregaVrbancic, @Benjamin-Lee Archive: 10.5281/zenodo.2800317

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/c44313ada36f12eebbaff10eb0888071"><img src="http://joss.theoj.org/papers/c44313ada36f12eebbaff10eb0888071/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/c44313ada36f12eebbaff10eb0888071/status.svg)](http://joss.theoj.org/papers/c44313ada36f12eebbaff10eb0888071)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@GregaVrbancic & @Benjamin-Lee, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @cMadan know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @GregaVrbancic

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @Benjamin-Lee

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @GregaVrbancic, it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

PDF failed to compile for issue #1277 with the following error:

/app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/lib/whedon.rb:83:in check_fields': Paper YAML header is missing expected fields: affiliations, bibliography (RuntimeError) from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/lib/whedon.rb:69:ininitialize' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/lib/whedon/processor.rb:32:in new' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/lib/whedon/processor.rb:32:inset_paper' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/bin/whedon:55:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/bin/whedon:116:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

casperdcl commented 5 years ago

@cMadan @GregaVrbancic, @Benjamin-Lee please use the tqdm:devel branch for review, I can commit any suggested changes there and make a new release before (hopeful) acceptance

casperdcl commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

cMadan commented 5 years ago

@GregaVrbancic @Benjamin-Lee, how is the review coming?

GregaVrbancic commented 5 years ago

@cMadan I'm planning to complete the review later today. Sorry for a bit of delay, due to the work stuff.

GregaVrbancic commented 5 years ago

So, here is my review of this submission. In general, the package seems really nice, but in order to publish this paper I have some minor suggestions or remarks:

  1. Stephen Larroque appears to be a significant contributor. Have you considered adding him in the authors' list?
  2. I have read the discussion about the scope of the paper and the target audience in #1258. My opinion is that the target audience could be more explicitly defined, but if @cMadan is fine with the current state, I will not insist on making any changes.
  3. In the references list, the reference with id tqdm-ar has release year and publisher defined differently as here.
  4. The iterable-based usage example featuring trange is not working out of the box. It would be nice if the example would also have the import statement for trange defined.
  5. Some guidelines on reporting issues or seeking support could be added.

@casperdcl please take a look at the listed remarks.

@cMadan I have a question regarding the whedon check references command. Is it possible to run it against the devel branch? Maybe in the same manner as whedon generate pdf from branch devel?

casperdcl commented 5 years ago

thanks @GregaVrbancic; will respond in more detail later.

  1. Stephen (@lrq3000), I know you haven't contributed in years but would be great to have you back on the team
  2. Any suggestions?
  3. Will check
  4. Will update
  5. Will add links to relevant documentation
Benjamin-Lee commented 5 years ago

Initial thoughts on the paper:

  1. If @arfon and @cMadan are happy with tqdm being in JOSS's scope, then I have no issues. To my mind, the more peer reviewed published software, the better.
  2. The structure of the paper requires work. Specifically, I would move the popularity section to the introduction and convert it to prose from bullets and eliminate the "see also" and "references in blogs" sections. JOSS papers generally use a single "summary" section. I would suggest attempting to do the same if at all possible. If you think you can't make it work in one section, this paper is a very good example of a well structured but complex JOSS paper. In general, I would suggest collapsing the bullets to paragraphs (with the exception of the first set in the summary, which I think works well)
  3. I don't think there's any need for the logo in the paper.
  4. The third sentence, explaining the meaning of the tqdm, is unnecessary.
  5. The bibliography seems to have a lot of redundant information. To start, I would suggest collapsing the GitHub references down to one.
  6. In the license section, I would remove the wording about citing tqdm on Zenodo since it'll be cited in JOSS!

Overall, tqdm is a great tool with wide applicability to both research and general software development. The repo is quite well organized and the code well documented. If I were to choose an area for improvement in the repo (though there isn't really much), I would suggest more developer documentation describing the structure of the tqdm/tqdm directory, since some of the files such as _main.py __main__.py are similarly named.

casperdcl commented 5 years ago

Full Response

Firstly, thank you @GregaVrbancic and @Benjamin-Lee (and @cMadan) for your time and reviews.

Review 1 (@GregaVrbancic)

  1. Stephen Larroque appears to be a significant contributor. Have you considered adding him in the authors' list?

  2. I have read the discussion about the scope of the paper and the target audience in #1258. My opinion is that the target audience could be more explicitly defined, but if @cMadan is fine with the current state, I will not insist on making any changes.

    • Any suggestions as to how the audience could be more explicitly defined? On PyPI, the intended audience is listed (based on https://pypi.org/pypi?%3Aaction=list_classifiers) - I can mirror this in the paper if you'd like.

    • In my opinion, a brief description of the target audience (based on https://pypi.org/pypi?%3Aaction=list_classifiers) would be useful and would improve the overall quality of the paper. Keep in mind that not all of the JOSS audience will go through the classifiers listed on PyPI package site.

    • Thanks, I have appended a description to the Introduction.

  3. In the references list, the reference with id tqdm-ar has release year and publisher defined differently as here.

  4. The iterable-based usage example featuring trange is not working out of the box. It would be nice if the example would also have the import statement for trange defined.

  5. Some guidelines on reporting issues or seeking support could be added.

    • Does this comment refer to the repository or the paper itself? If it's the repository, there's https://github.com/tqdm/tqdm/#contributions or https://tqdm.github.io/contributing/.

    • Yes, I was refe[r]ring to the repository. Look at the review checklist, sect[i]on Documentation, subsection Community guidelines. You have ad[d]ressed the the 1) nicely in the documentation, but I was aiming more toward the points 2) and 3), or have I missed something?

    • There's https://github.com/tqdm/tqdm#faq-and-known-issues which mentions GitHub-Issues. I have now updated the latter to point to all issues (including closed ones and PRs), and added both links to the website (https://tqdm.github.io/) now so they're harder to miss. N.B. anyone attempting to file a new issue/PR is presented with a message:
    • [ ] I have visited the source website, and in particular read the known issues
    • [ ] I have searched through the issue tracker for duplicates
    • [ ] I have mentioned version numbers, operating system and environment, where applicable: 
      import tqdm, sys
print(tqdm.__version__, sys.version, sys.platform)
    • [ ] If applicable, I have mentioned the relevant/related issue(s)

General Comments

Review 2 (@Benjamin-Lee)

  1. If @arfon and @cMadan are happy with tqdm being in JOSS's scope, then I have no issues. To my mind, the more peer reviewed published software, the better.

    • If there are other journal(s) you can recommend, I'd be happy to submit there instead if you feel it's more appropriate. I think tqdm is one of the world's most used Python packages (PyPI-Downloads and Libraries-Rank; usually on the first page of https://libraries.io/search?order=desc&platforms=PyPI&sort=rank) because its scope is pretty large.

  2. The structure of the paper requires work. Specifically, I would move the popularity section to the introduction and convert it to prose from bullets and eliminate the "see also" and "references in blogs" sections. JOSS papers generally use a single "summary" section. I would suggest attempting to do the same if at all possible. If you think you can't make it work in one section, this paper is a very good example of a well structured but complex JOSS paper. In general, I would suggest collapsing the bullets to paragraphs (with the exception of the first set in the summary, which I think works well)

    • I've endeavoured to reorder, make prosaic, and eliminate as requested.

  3. I don't think there's any need for the logo in the paper.

    • Removed.

  4. The third sentence, explaining the meaning of the tqdm, is unnecessary.

    • I feel etymological explanation/justification of an otherwise seemingly random combination of letters is important. The other reviewer was kind enough to point out a mistake in the reference here. I'm happy to leave this to the editor to decide.

  5. The bibliography seems to have a lot of redundant information. To start, I would suggest collapsing the GitHub references down to one.

    • I tried to keep the reference count down. Unfortunately I can't see where I can merge two references as they are all unique citations for specific claims.

  6. In the license section, I would remove the wording about citing tqdm on Zenodo since it'll be cited in JOSS!

    • Reworded.

General Comments

Overall, tqdm is a great tool with wide applicability to both research and general software development. The repo is quite well organized and the code well documented. If I were to choose an area for improvement in the repo (though there isn't really much), I would suggest more developer documentation describing the structure of the tqdm/tqdm directory, since some of the files such as _main.py __main__.py are similarly named.

Thank you. I intend tqdm as a gold-standard reference for repository management/framework, and thus would be very willing to make any suggested changes to make things clearer. I'm sure you've seen https://github.com/tqdm/tqdm/#contributions or https://tqdm.github.io/contributing/.

Regarding the layout, https://github.com/tqdm/tqdm/pull/198#issuecomment-394184136 is a heavily revised comment in a milestone v5 release that's past due by about 2 years due to a personal lack of time! I will incorporate that comment (likely heavily revised) into the developer documentation when that issue is eventually addressed.

casperdcl commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

casperdcl commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

casperdcl commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

casperdcl commented 5 years ago

Looks ready to me for a re-check now. Will merge into master if it's ok.

GregaVrbancic commented 5 years ago

@casperdcl please consider the following proposed improvements.

  1. I have read the discussion about the scope of the paper and the target audience in #1258. My opinion is that the target audience could be more explicitly defined, but if @cMadan is fine with the current state, I will not insist on making any changes.

In my opinion, a brief description of the target audience (based on https://pypi.org/pypi?%3Aaction=list_classifiers) would be useful and would improve the overall quality of the paper. Keep in mind that not all of the JOSS audience will go through the classifiers listed on PyPI package site.

  1. Some guidelines on reporting issues or seeking support could be added.

Yes, I was refering to the repository. Look at the review checklist, secton Documentation, subsection Community guidelines. You have adressed the the 1) nicely in the documentation, but I was aiming more toward the points 2) and 3), or have I missed something?

My other remarks you've addressed properly in my opinion.

casperdcl commented 5 years ago

Thanks @GregaVrbancic. I've update my response above (but also explicit the changes below):

  1. In my opinion, a brief description of the target audience (based on https://pypi.org/pypi?%3Aaction=list_classifiers) would be useful and would improve the overall quality of the paper. Keep in mind that not all of the JOSS audience will go through the classifiers listed on PyPI package site.

    • Thanks, I have appended a description to the Introduction.

  2. Yes, I was refe[r]ring to the repository. Look at the review checklist, sect[i]on Documentation, subsection Community guidelines. You have ad[d]ressed the the 1) nicely in the documentation, but I was aiming more toward the points 2) and 3), or have I missed something?

    • There's https://github.com/tqdm/tqdm#faq-and-known-issues which mentions GitHub-Issues. I have now updated the latter to point to all issues (including closed ones and PRs), and added both links to the website (https://tqdm.github.io/) now so they're harder to miss. N.B. anyone attempting to file a new issue/PR is presented with a message:
    • [ ] I have visited the source website, and in particular read the known issues
    • [ ] I have searched through the issue tracker for duplicates
    • [ ] I have mentioned version numbers, operating system and environment, where applicable: 
      import tqdm, sys
print(tqdm.__version__, sys.version, sys.platform)
    • [ ] If applicable, I have mentioned the relevant/related issue(s)
casperdcl commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

cMadan commented 5 years ago

@casperdcl @GregaVrbancic @Benjamin-Lee, just to follow-up re: scope of work and JOSS. I appreciate both reviewers commenting on this--I do agree that the project is on the edge of JOSS' scope (and had brought this up in the pre-review). However, we have published tools that are similarly tools for researchers/development (rather than researcher end-users) and tdqm has clearly demonstrated it's utility as a development tool, so we are considering it within-scope here. I think the statement of need provided in the paper is sufficient, but suggestions to improve it are, of course, welcome.

GregaVrbancic commented 5 years ago

@cMadan thank you for the follow-up. I think that @casperdcl has addressed all my remarks, so from my standpoint, the paper could proceed to publish.

cMadan commented 5 years ago

@GregaVrbancic, thank you for the thorough review!

@Benjamin-Lee, please look over the repo again when you have a chance and let me know what you think.

cMadan commented 5 years ago

@Benjamin-Lee, could you take a quick look to see if your last comments were suitably addressed? Thanks!

Benjamin-Lee commented 5 years ago

One last tiny change and then I think it's good to go:

within a Command-line interface

Should this be CLI (since you defined the acronym in the previous section) or within a command-line interface with a lowercase c>

Either way, congrats @casperdcl!

casperdcl commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

casperdcl commented 5 years ago

Thanks @Benjamin-Lee @GregaVrbancic.

@cMadan let me know what the next steps are - ideally I'd like to merge into master before official publication.

casperdcl commented 5 years ago

erm ping @cMadan

labarba commented 5 years ago

@casperdcl, FYI, I have followed up with the editor, and he is on travel but will update us soon.

cMadan commented 5 years ago

@whedon generate pdf from branch devel

whedon commented 5 years ago 
Attempting PDF compilation from custom branch devel. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

cMadan commented 5 years ago

@casperdcl, everything looks good and I'm almost ready to accept. Let me know after you merge into master and have the new GitHub release and new Zenodo DOI minted, then I'll finalise things here.

casperdcl commented 5 years ago

@whedon generate pdf from branch master

whedon commented 5 years ago 
Attempting PDF compilation from custom branch master. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

casperdcl commented 5 years ago

@cMadan all done: merged, tagged (v4.32.0) and released on all channels (GitHub/Zenodo/PyPI/Conda/DockerHub/Snapcraft). https://github.com/tqdm/tqdm#tqdm for all relevant badges/links.

casperdcl commented 5 years ago

note that this is more recent than the version mentioned in the original (https://github.com/openjournals/joss-reviews/issues/1277#issue-413844605)

casperdcl commented 5 years ago

correction just released v4.32.1 with a minor bugfix

cMadan commented 5 years ago

@whedon set 10.5281/zenodo.2800317 as archive