search

openjournals / joss-reviews

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

[REVIEW]: hammurabi X: a C++ package for simulating Galactic emissions #1889

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @gioacchinowang (Jiaxin Wang) Repository: https://bitbucket.org/hammurabicode/hamx Version: v2.3.1.1 Editor: @dfm Reviewer: @zonca, @pqrs6 Archive: 10.5281/zenodo.3687352

Status

status

Status badge code:

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

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) by leaving comments 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

@zonca & @pqrs6, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @dfm know.

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

Review checklist for @zonca

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @pqrs6

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @zonca, @pqrs6 it looks like you're currently assigned to review 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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:377:in parse': (tmp/1889/joss/paper.md): did not find expected key while parsing a block mapping at line 2 column 1 (Psych::SyntaxError) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:377:inparse_stream' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:325:in parse' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:252:inload' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:473:in block in load_file' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:472:inopen' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:472:in load_file' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon.rb:115:inload_yaml' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon.rb:85:in initialize' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon/processor.rb:36:innew' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon/processor.rb:36:in set_paper' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/bin/whedon:55:inprepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/bin/whedon:116:in <top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in `

'

dfm commented 4 years ago

Just a quick reminder that this has been submitted in collaboration with AAS. AAS makes a small donation to JOSS to support this process. For more information check out the following links and let me know if you have any issue with this:

dfm commented 4 years ago

@gioacchinowang: I've opened a pull request on your repo that fixes the formatting of the paper so that it compiles.

gioacchinowang commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

gioacchinowang commented 4 years ago

I've done a slight modification about the JOSS author, so @whedon generate pdf

gioacchinowang commented 4 years ago

@gioacchinowang: I've opened a pull request on your repo that fixes the formatting of the paper so that it compiles.

Thanks a lot! and I (and all the co-authors) agree with the publishing terms.

gioacchinowang commented 4 years ago

@dfm dear editor, we are having a slightly different author list in this JOSS paper and the AAS paper. As I'm also asking the editorial office at the AAS, does a joint publication require the same author list on both papers?

arfon commented 4 years ago

@gioacchinowang - we don’t have a policy against this so I think it’s ok for them to be different.

gioacchinowang commented 4 years ago

@gioacchinowang - we don’t have a policy against this so I think it’s ok for them to be different.

great! thanks for the message :)

dfm commented 4 years ago

@zonca, @pqrs6: I just wanted to check in to see how things are going with this review. Let me know if you have any questions!

zonca commented 4 years ago

@dfm I am at "Installation", https://bitbucket.org/hammurabicode/hamx/issues/8/joss-install-instructions is there an expected time-frame to have the review completed?

dfm commented 4 years ago

@zonca: whoops! Sorry - didn't realize there was a conversation going on over there (I'm used to them being on GitHub). We don't have a specific time line here, but we don't like things to go quiet for more than a week - that's why I was checking in. I'll keep an eye on the issues over there.

dncnwtts commented 4 years ago

I'm on the install stage as well, although have a bit less experience with C++ installations so have some difficulties. Sorry for the delay.

On Thu, Nov 28, 2019 at 7:39 AM Dan Foreman-Mackey notifications@github.com wrote:

@zonca https://github.com/zonca: whoops! Sorry - didn't realize there was a conversation going on over there (I'm used to them being on GitHub). We don't have a specific time line here, but we don't like things to go quiet for more than a week - that's why I was checking in. I'll keep an eye on the issues over there.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1889?email_source=notifications&email_token=ABRNSOIAUS3RCNH7FUW7YYDQV63ZRA5CNFSM4JM4KKP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFMPVUQ#issuecomment-559479506, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRNSOKYNJK6BJWHA2HB4XLQV63ZRANCNFSM4JM4KKPQ .

dncnwtts commented 4 years ago

I've been able to install and run Hammurabi X and recommend publication with a few minor issues.

  1. There is a bug in the python wrapper. The executable path should not need to be set by default, but I found that I had to call the code as follows; sim = hpx.Hampyx(xml_path=xmlpath, exe_path=exe_path) where exe_path was set to my own hamx executable.
  2. A potentially related issue related to my own inexperience with C++, I had to set the LD_LIBRARY_PATH in my jupyter notebook to run the tutorial script %env LD_LIBRARY_PATH=/home/dwatts/software/hamx/lib:/home/dwatts/software/cfitsio-3.47/lib, a line that wasn't necessary when running hamx from bash.
  3. I didn't see any clear instructions for people wanting to contribute. I found a small typo in the python wrapper documentation and made a commit directly in bitbucket, so I don't think this is a show-stopper.
  4. I didn't see a LICENSE file in the bitbucket repository, which should be added before publishing.
  5. It's possible I don't understand the JOSS guidelines, but it seems the references in the pdf proof don't conform with https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html#citation_syntax.
  6. In tutorial_01.ipynb's cell [18], running the simulation step after setting a mask fails, with an error terminate called after throwing an instance of 'std::runtime_error' what(): no mask file. It would be good to address this.

All in all, considering that I suck at compilers, this was a pretty painless process. My only suggestion would be a python wrapper with more explanations of the details, but the fact that the tutorial notebooks reproduced figures in https://arxiv.org/abs/1907.00207 was an excellent indication that I had done what the authors had intended.

dfm commented 4 years ago

@pqrs6 The citations look fine to me - can you give a specific example of what is inconsistent? Thanks!

dncnwtts commented 4 years ago

@dfm I think I'm confused by the last bullet point in the author checklist:

"References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?"

I guess the proper syntax link might be referring to just pandoc and not the JOSS standards, but it does specify square brackets. The citations look fine to me in the proof, just not what's specified in the bullet points.

dfm commented 4 years ago

@pqrs6: The square brackets need to be in the markdown file but those are automatically converted to the ones you see in the proof when the pdf is generated.

gioacchinowang commented 4 years ago

@pqrs6 sorry for my late reply, in terms of the LICENSE, it is the COPYING file, we can rename it if you think it is necessary. In terms of the other issues related to the jupyter notebook, I'm working on it, hopefully I can solve them in a few days. Previously, I run the notebooks only on the docker image, and so didn't realize there are so many issues.

dncnwtts commented 4 years ago

@gioacchinowang I think it would be good to rename the COPYING file, just to conform with standards.

Thanks!

gioacchinowang commented 4 years ago

@pqrs6 Sure, done, and I've integrated the jupyter notebook tests (with pytest and nbval plugin) into the bitbucket pipeline as Andrea suggested, now at least everything is fine in the docker image, and the issue #1 is solved, #6 is due to my fault by using absolute file path instead of the relative one, already fixed that.

gioacchinowang commented 4 years ago

@pqrs6 btw, we have a wiki page for the python wrapper, https://bitbucket.org/hammurabicode/hamx/wiki/wrapper/hampyx.md

zonca commented 4 years ago

ok, software and docs are fine, now sent feedback on paper, we are almost done.

zonca commented 4 years ago

@gioacchinowang well done, @dfm paper accepted

For reference, relevant issues we worked through:

gioacchinowang commented 4 years ago

@dfm @zonca @pqrs6 Thank you all for reviewing and suggestions, learned a lot through this process :)

dfm commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

dfm commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Attempting to check references...
whedon commented 4 years ago 

OK DOIs

- 10.1088/1361-6382/aacde0 is OK
- 10.1051/0004-6361:200810564 is OK

MISSING DOIs

- None

INVALID DOIs

- None
dfm commented 4 years ago

@zonca @pqrs6: Thanks for your constructive reviews!

@gioacchinowang: Two minor comments on the paper:

After you make these changes, we will pause this review until you get the DOI for your AAS Journals publication. @arfon and @crawfordsm: let us know if there's anything else we need to do to synchronize these submissions.

gioacchinowang commented 4 years ago

@dfm Thanks! Done.

gioacchinowang commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

labarba commented 4 years ago

@gioacchinowang — do update us on the status/timeline of your AAS publication, when opportune.

gioacchinowang commented 4 years ago

@labarba The AAS publication is under the final revision, and the editor expect it to be done within a few days. I have been asked to include the JOSS reference and DOI in the resubmition. Thanks for asking!

danielskatz commented 4 years ago

And similarly, the JOSS paper will include the AAS reference and DOI

dfm commented 4 years ago

@gioacchinowang the DOI for the JOSS publication will be: https://doi.org/10.21105/joss.01889 Once your AAS paper has been accepted and you have a DOI, let us know, and we'll do the same. Thanks!

gioacchinowang commented 4 years ago

@dfm @danielskatz @labarba Dear all, the DOI of the ApJS article is: 10.3847/1538-4365/ab72a2

dfm commented 4 years ago

@gioacchinowang: Perfect. Can you update the aas-doi and aas-journal arguments in the paper metadata then we should be good to go. Thanks!

gioacchinowang commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

gioacchinowang commented 4 years ago

@dfm Thanks! I've updated that, please check :)

arfon commented 4 years ago

@dfm - this looks good. Just an FYI that the AAS DOI doesn't resolve yet, and likely won't for another few weeks. We can either hold off publishing this paper until then or publish now with a broken link in the JOSS PDF. I'll leave that up to you and @gioacchinowang.

dfm commented 4 years ago

I think it's probably best to wait unless @gioacchinowang would really like this published before. I'll keep checking back, but @gioacchinowang perhaps you can let us know when the AAS article is published (with the link)? The JOSS workflow gets the papers published much faster so we can get a more or less simultaneous publication.