[REVIEW]: unyt: Handle, manipulate, and convert data with units in Python

whedon commented 6 years ago

Submitting author: @ngoldbaum (Nathan Goldbaum) Repository: https://github.com/yt-project/unyt Version: v1.0.3 Editor: @kyleniemeyer Reviewer: @Cadair, @tbekolay, @ygrange Archive: 10.5281/zenodo.1344605

Status

Status badge code:

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

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

@Cadair & @tbekolay & @ygrange, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
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 @kyleniemeyer know.

Review checklist for @Cadair

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v1.0.3)?
[x] Authorship: Has the submitting author (@ngoldbaum) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @tbekolay

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v1.0.3)?
[x] Authorship: Has the submitting author (@ngoldbaum) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @ygrange

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v1.0.3)?
[x] Authorship: Has the submitting author (@ngoldbaum) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[ ] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @Cadair, 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:

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

watching

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 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

kyleniemeyer commented 6 years ago

👋 @ngoldbaum @Cadair @tbekolay @ygrange

tbekolay commented 6 years ago

I went through the checklist and all was great, except for the "Does the release version given match the GitHub release (v1.0.3)?" checkbox. There has been a new release since the paper was submitted (it is now at v1.0.4), but this does not seem to me to be an issue.

This is my first JOSS review, so apologies for the newb questions, but is filling out checklist all that is required for the review? I.e., publication is based on these yes/no metrics and not any other criteria, such as the quality of the paper itself?

As long as those kinds of general comments are not unwelcome, then I'd like to commend @ngoldbaum and the other authors on an excellently written paper, and the whole unyt development team on a well documented and well organized codebase. I'm very pleased to see that yt units has been extracted to a separate project with minimal dependencies and am optimistic that it can become the de facto standard for Python units handling :+1:

kyleniemeyer commented 6 years ago

@tbekolay thank you! The version mismatch is fine, we recognize the software will change after submission (but hopefully not substantially).

Yes, we do ask that you fill out the checklist—that, plus the resolution of any issues you may bring up, is how we determine that our acceptance criteria is met. There aren't any additional criteria, though any suggestions/comments on the paper itself (or any other part of the software) are welcome.

ygrange commented 6 years ago

Ok, I finally got around to have a look at this. I'll try to submit some comments half-way next week.

ygrange commented 6 years ago

The main comment I have is: Great paper, I really enjoyed reading it! You did a really good job at analysing the differences in performance and functionality between unyt and the two comparable packages. I will go through the checklist shortly, but let me post my comments on the contents first:

First a comment on the code base: Your code coverage is really great! There is no documentation for users to execute the unit tests after installation from source. It's probably one extra line to the readme, but would certainly be great to have.

On the text then: page 7: “for each of the benchmarks below” -> not all benchmarks are below this text so it’s a bit confusing. Rephrasing to "in this section" or something similar fixes this.

Let me repeat that I really love the thorough profiling here. Very impressive and very nice to read!

I have a few comments

Figure 2: I don’t really get why the overhead is negative for square root of the largest arrays. Probably the effect is similar to what you discuss for figure 3 later.
discussion of figure 3 and 4:
- Using the phrase “overhead of ~0.5” is a bit confusing because my brain parses that as a 150% effect i.s.o. 50% which is meant. Also I find it a bit hard to parse what is exactly meant here. What really helped me was to realise that the effect is only present in figure 3 (different units) and not 4 (equal units). I believe that explicitly mentioning this can make it a bit easier to grasp.
- The different handling of divisions essentially means that in astropy and Pint 1kg / (1kg) = 1kg while in unyt the answer is 1 (dimensionless), right? It may be illustrative to put down an example like this in the text.
In the description of ufunc you talk about figure 4 and 5. Should be 5 and 6. (a bit futher Figure 4 should be Figure 5. Also, I assume the abovementioned effect is also present in the ufunc versions.

ygrange commented 6 years ago

On the dependencies: to do the testing, one needs to install a few extra dependencies.

ygrange commented 6 years ago

Documentation: the docstrings look really good and complete. Even though they are quite descriptive by nature I would advice to add a desciption to the exception classes (i.e. if I use unyt in my work, what case is covere by what exception?)

ygrange commented 6 years ago

I see the list of authors in the paper.md file, but there is no author listed in the PDF. Is this a technical problem, or is this actually always the case? (question for the @kyleniemeyer I guess).

Done spamming for now 😄 .

ngoldbaum commented 6 years ago

Awesome, thank you for the comments. @kyleniemeyer how should I address the comments to the paper? In a pull request to unyt that I ping @ygrange on?

Cadair commented 6 years ago

This is a really nice paper, with a great comparison between the three packages. The quality of the code, docs and tests all looks fantastic as I would've expected :smile:. I hope that this package gains a lot of traction by becoming such a lightweight standalone package (something I have thought astropy.units should do for a long time). I look forward to using it in some of my own code in the future.

One small comment on the paper:

In the section on "Unary arithmetic operations" there is a line that reads "the y-axis is plotted on a log scale", should this not be "x-axis"?

kyleniemeyer commented 6 years ago

@ngoldbaum you can make a PR or issues over on your repo that reference this one, so that they'll show up in this thread.

kyleniemeyer commented 6 years ago

@ygrange regarding your question about the authors, I do see the authors right below the title in the PDF—do you see something else?

ygrange commented 6 years ago

Ugh, sorry. This is also my second review ever over here so I just discovered the paper in the repo is not the same (certainly in terms of layout) as the proof-read version over here. Whoops. Thanks for pointing that out!

ngoldbaum commented 6 years ago

Ah yes, sorry if that was confusing! It was convenient to have a draft PDF in the repo before I submitted to JOSS. I'll replace it with the final version once the review process is finished.

ngoldbaum commented 6 years ago

I've gone ahead and opened https://github.com/yt-project/unyt/pull/41, https://github.com/yt-project/unyt/pull/42, https://github.com/yt-project/unyt/pull/43, and https://github.com/yt-project/unyt/pull/44. I'd very much appreciate eyes from any of the reviewers on those pull requests.

@kyleniemeyer once those PRs are merged I'd like to cut a new release and update the metadata json files to reflect the updates from the reviewers. Is there anything else I should do while I'm doing that or do you think I've satisfied all the issues the reviewers pointed out?

ygrange commented 6 years ago

Nothing to add. Thanks for the changes! I would be very happy if this one would be published.

kyleniemeyer commented 6 years ago

@ngoldbaum that plan sounds good to me. Just ping me here when you have finalized the paper edits, and I will take a final look from an editing perspective. After I give the 👍 on that, you'll need to archive the final repo (software + data) in Zenodo (e.g.) and provide the DOI back here.

ngoldbaum commented 6 years ago

@kyleniemeyer I've pushed unyt 1.0.5 to pypi and conda-forge. Can you take a look? If you approve I'll put that version on zenodo.

kyleniemeyer commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

kyleniemeyer commented 6 years ago

@ngoldbaum OK, I have some edits for the paper:

Please add more complete location information to the affiliations (e.g., city, state, country)
First sentence in 2nd paragraph: capitalize "Python". Also, in the 7th paragraph.
Please replace the hyphens used in the 2nd, 3rd, and 7th paragraphs with proper em-dashes (i.e., — vs -)
"h5py" should probably also be in texttt format to match the rest of the software. Also, ndarray in the 4th paragraph and in the Pint section.
I think there is a typo in this sentence of the 3rd paragraph: "In addition, unyt ships with a number of other useful predefined unit systems based, including ..." (emphasis mine). The list structure in the second have of this sentence might need another look as well, as I think you have multiple lists that all use commas—if that is correct, you should use semicolons for the main list
First sentence of the "NumPy ufunc performance" section: "Lastly, In Figures..." -> "Lastly, in Figures..."
Reference Croton 2013: is this an article? It is missing the journal name (or equivalent)
Reference Pontzen 2013: missing a URL or DOI
The Astropy Collaboration reference is missing the arXiv identifier and link (I recommend citing in this format: https://arxiv.org/help/faq/references)

After you make these changes, please archive the entire repo to Zenodo and report back the DOI

ngoldbaum commented 6 years ago

Thanks for the detailed copy-editing comments. My grandmother was an editor before she retired and helped me many times over the years, it always makes me really appreciative of careful copy-editing.

My bibtex entries were taken from the NASA ADS, which is usually the one-stop shop for astronomers who are doing bibliographic searches. It would be nice if we could make it so it generated bibtex that played nicely with JOSS. I think they'd like they might like the feedback about adding a URL to the arxiv entry directly to the bibtex link. Perhaps @augustfly might know the appropriate person to contact about that?

It also took me a little while to figure out how to archive on Zenodo - I wish the docs on github's release functionality were clearer that it works for existing releases! Anyway, here is the zenodo link:

https://zenodo.org/record/1344605

kyleniemeyer commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

augustfly commented 6 years ago

Hi,

I'm not sure what is being asked of me?

Looking at the last version of the bibtex before the last commit, and at the JOSS author guidelines, the only problem I see is that pynbody wasn't a recent ADS bibtex entry.

The guidelines --

https://joss.readthedocs.io/en/latest/submitting.html#example-paper-and-bibliography

suggest ADS macros (e.g., \pasa) are supported as are eprint + archiveprefix keys, which is how to encode both arXiv and ASCL entries into anchored URLs (as described here: https://arxiv.org/hypertex/bibstyles/).

FWIW, howpublished is a bibtex hack that I hope isn't part of anyone's workflow. :-) Not that I'm not one to avoid bibtex hacks, having added version tags to our Journal's preferred bibtex styles for citing software.

having said all that -- the person you want is @aaccomazzi

ngoldbaum commented 6 years ago

Hi August,

Nothing being asked of you in particular, just pinging you because I thought it was highly likely you'd have more context than I would. Apologies for the imposition, that wasn't my intention.

Thanks for the context :)

Then I guess the issue is that JOSS or whedon isn't picking up on the eprint tags or ADS macros, or at least they're not ending up as nicely rendered journal names (in some cases anyway) or URLs in the citation list of a JOSS paper PDF.

aaccomazzi commented 6 years ago

Just checked the generated PDF and saw no problems with missing links in the reference, so I assume the issue has been fixed by the authors. If there is a systematic issue with ADS reference generation please file an issue with full details here: https://github.com/adsabs/export_service

augustfly commented 6 years ago

No worries. I wasn't upset, Nathan -- this is one of my favorite subjects! I can't rant about howpublished all day long.

I simply don't see how the JOSS author instructions can use ADS bibtex verbatim but that something needs to change with ADS bibtex for it to be compiled correctly.

kyleniemeyer commented 6 years ago

@whedon set 10.5281/zenodo.1344605 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1344605 is the archive.

kyleniemeyer commented 6 years ago

@arfon this paper is now accepted and ready to be published!

kyleniemeyer commented 6 years ago

@ngoldbaum @aaccomazzi @augustfly Arfon is definitely the person to include in the discussion of how we process bibtex... I'm not even sure if we are using biblatex or bibtex. However, in my experience I have never not had to customize a .bib file, for JOSS or otherwise :)

arfon commented 6 years ago

@Cadair, @tbekolay, @ygrange - many thanks for your reviews here and to @kyleniemeyer for editing this submission ✨

@ngoldbaum - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00809 ⚡️:rocket: :boom:

whedon commented 6 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.00809/status.svg)](https://doi.org/10.21105/joss.00809)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.00809">
  <img src="http://joss.theoj.org/papers/10.21105/joss.00809/status.svg" alt="DOI badge" >
</a>

This is how it will look in your documentation:

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=

openjournals / joss-reviews