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

[REVIEW]: Gala: A Python package for galactic dynamics #388

Closed
whedon opened this Issue Sep 2, 2017 · 30 comments

Comments

Projects
None yet
4 participants
@whedon
Copy link
Collaborator

whedon commented Sep 2, 2017

Submitting author: @adrn (Adrian Price-Whelan)
Repository: https://github.com/adrn/gala
Version: v0.2.1
Editor: @arfon
Reviewer: @crawfordsm
Archive: 10.5281/zenodo.1004642

Status

status

Status badge code:

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

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 questions

@crawfordsm, please carry out your review in this issue by updating the checklist below (please make sure you're logged in to GitHub). The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arfon know.

Conflict of interest

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

Code of Conduct

General checks

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

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • 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

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

This comment has been minimized.

Copy link
Collaborator

whedon commented Sep 2, 2017

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS. @crawfordsm it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐️ Important ⭐️

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS 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
@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 2, 2017

@crawfordsm - please carry out your review in this issue by updating the checklist above and giving feedback in this issue. The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines

Any questions/concerns please let me know.

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 13, 2017

Friendly reminder on this @crawfordsm 😁

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 13, 2017

Sorry about that -- am I suppose to be able to check or edit the comment at the top?

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 13, 2017

Sorry about that -- am I suppose to be able to check or edit the comment at the top?

Yep, you should be able to edit the checklist at the top of the issue.

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 13, 2017

Ah that's not the case. Unfortunately, I can't edit the top comment. Happy to copy and past and use another comment though.

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 13, 2017

Ah that's not the case. Unfortunately, I can't edit the top comment. Happy to copy and past and use another comment though.

Sure, that works too.

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 13, 2017

Conflict of interest

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

Code of Conduct

  • I confirm that I read and will adhere to the JOSS code of conduct.

General checks

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

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • 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

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

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 13, 2017

Using this as notepad for high level comments -- will transfer to the repository once finished with review

The gala software package provides tools for galactic dynamic measurements. It is python package that wraps lower level, higher performance, and provides a user friendly interface to those lower level codes. It is well integrated with astropy, and provides documentation of its functionality. The code is generally useful to the astronomical community in both terms of research and education.

  • Contributors are listed in the Zenodo DOI and on the README with how to cite. Primary author is listed in paper.md, but not contributing authors. Might be useful to have a AUTHORS.md in the main repository

  • The one weakness of the documentation is really a high level justification of the project. I found this information in the paper.md and I would suggest including this material in the main documentation.

Minor comments:

  • 'why' page might be better titled 'Benefits of Gala'

adrn added a commit to adrn/gala that referenced this issue Sep 14, 2017

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 14, 2017

Thanks for the comments! I addressed the ones you left so far:

  • I added an AUTHORS.rst file, and now include it in the documentation
  • I added the high-level justification / context to the main page of the documentation
  • I renamed the 'why' page
@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 19, 2017

Comments above have been address.

The functionality and performance have been tested as well and they match the claims that have been reported. Overall, the package is well documented with good examples allowing example problems to be reported.

Final comments to be address:

  • It would be useful to add testing information to the installation page

  • it would be good to indicate whether contributions are welcome and the best way to make them. Seeking support and asking questions is clearly indicated but how to make contributions is not. Perhaps a contributing section in the README would help.

Suggestion for the future: It might be good to include a gallery page including further examples.

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 23, 2017

Good ideas!

http://gala-astro.readthedocs.io/en/latest/

  • I added a link and page "Running the tests" linked from the main docs
  • I added a section "How to contribute" to the main docs page

I like the idea of having a gallery for examples / tutorials - I'll explore that when I've written some more content.

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 24, 2017

Looks great to me! I noticed the other authors aren't listed in the paper.md file. Last thing needed to updated for the checklist to be complete.

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 24, 2017

That was actually intentional - is there a requirement that all contributors are authors?

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 24, 2017

(If not, I would prefer to keep it as is)

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 24, 2017

Seems like a question for @arfon, but JOSS does seem to ask to include the list of Authors of the software in the authors.

If the JOSS entry will be the main citation for the software, then I'd suggest including them as you do currently include them under 'how to cite' section. I could understand having other justifications and it would be good to hear them though this is likely up to @arfon

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 25, 2017

That was actually intentional - is there a requirement that all contributors are authors?

We ask reviewers to check that the list of authors is 'correct' but this is ultimately somewhat subjective based on what seems reasonable. See this thread for a similar discussion

@adrn - I'm assuming you're not including the other three contributors as they all made relatively minor contributions to the package? If so, perhaps you could acknowledge their contributions in the README or somewhere in the body of the paper?

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 25, 2017

I'm assuming you're not including the other three contributors as they all made relatively minor contributions to the package?

Exactly

If so, perhaps you could acknowledge their contributions in the README or somewhere in the body of the paper?

Done - I added a link to the AUTHORS.rst file to the README, and added an acknowledgements section to the paper.

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 25, 2017

Thanks @adrn. @crawfordsm - can you confirm we're good to accept here?

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 28, 2017

Thanks for adding it in the acknowledgements. This is obviously a difficult question and one with out a very clearly defined answer. A statement from the JOSS board might be very helpful going ahead in the future also to give some ideas, and leadership, for the community as to what are the best suggested and most fair practices. I know I have had difficulty in the past in terms of balancing wanting to reflect what is >90% my own work along with the relatively small contributions of others and it is a difficult thing to balance.

The paper format is certainly outdated to properly attribute the work of different contributors as authors. As a suggestion to @arfon, I might suggest two parts to the attribution section of the paper.md file, which would be Authors and Contributors. There might be some overlap between the two, but authors would be defined as those that have made significant scientific/programming contribution to the code and contributors are people that have made more programmatic contributions to the code. The option or suggest to include a description of the contribution of each author might be helpful as well.

For @adrn , I think my concern is how are you going to suggest the paper is cited? Publications and citations are the credit of the academic world, and I like that your currently suggested manner of citations is the zenodo link that includes your contributors. If you were going to change this to being only the JOSS paper, I think I'd find that problematic as most of your contributors are academics. On the other hand, the contributions of the others are relatively minor to your overall development of the package. Personally, I err on the side of included more people, but I definitely understand arguments on the other side and could see the acknowledgement as being sufficient based on the level of contribution.

Nonetheless, as there is no definitive policy and this work is predominately yours, I'd default to what you thought best reflected the package and contribution of others.

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 28, 2017

I agree that this is a tough and confusing topic. For lack of a clear set of rules, I'll just default to following the same rules I use for other scientific papers: in my own arbitrary system (adapted from @davidwhogg), if someone contributes a "minor change" (yes, this is subjective), they are added to the acknowledgements and explicitly asked if they would like to be included as authors of the paper. If they say yes, they are expected to contribute more significantly to the paper. I do think there should be a threshold to receiving citation-based attribution for contributions in general.

Since there is no obvious analogy to the above in this case (the "paper" is not the main work effort; the codebase is), I'll ask if the contributors would like to be authors on the paper, and I'll be more lenient about expecting further contributions.

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 28, 2017

@adrn

This comment has been minimized.

Copy link

adrn commented Sep 28, 2017

(I gave them until 1 Oct to respond!)

@crawfordsm

This comment has been minimized.

Copy link
Collaborator

crawfordsm commented Sep 28, 2017

@adrn

This comment has been minimized.

Copy link

adrn commented Oct 1, 2017

I've heard from 2/3 committers (see: adrn/gala#100) - keep the author list as is. Sounds like this is ready to go then @arfon?

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Oct 4, 2017

@adrn - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

@arfon arfon referenced this issue Oct 6, 2017

Closed

[REVIEW]: Spectrum: Spectral Analysis in Python #348

18 of 18 tasks complete
@adrn

This comment has been minimized.

Copy link

adrn commented Oct 7, 2017

@arfon DOI is still catching up, but it should be here soon:
https://doi.org/10.5281/zenodo.1004642

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Oct 9, 2017

@whedon set 10.5281/zenodo.1004642 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator

whedon commented Oct 9, 2017

OK. 10.5281/zenodo.1004642 is the archive.

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Oct 9, 2017

@crawfordsm - many thanks for your review here!

@adrn - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00388 ⚡️ 🚀 💥

@arfon arfon closed this Oct 9, 2017

@arfon arfon added the accepted label Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment