search

openjournals / joss-reviews

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

[REVIEW]: SeisModels.jl: A Julia package for models of the Earth's interior #2043

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @anowacki (Andy Nowacki) Repository: https://github.com/anowacki/SeisModels.jl Version: v1.1.0 Editor: @kbarnhart Reviewers: @daniellivingston, @joa-quim Archive: 10.6084/m9.figshare.11993313

Status

status

Status badge code:

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

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

@daniellivingston, @fhorrobin, & @joa-quim 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 @kbarnhart know.

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

Review checklist for @daniellivingston

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @fhorrobin

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @joa-quim

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. @daniellivingston, @fhorrobin 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 
Reference check summary:

OK DOIs

- 10.1111/j.1365-246X.1995.tb03540.x is OK
- 10.1111/j.1365-246X.1991.tb06724.x is OK
- 10.1016/0031-9201(81)90046-7 is OK
- 10.1126/science.1199375 is OK
- 10.1785/gssrl.70.2.154 is OK
- 10.1785/gssrl.81.3.530 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

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

kbarnhart commented 4 years ago

:wave: @anowacki, @daniellivingston, @fhorrobin, @joa-quim this is the review thread for the paper. All of our communications will happen here from now on.

All reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/2043 so that a link is created to this thread. This helps create a record of the changes in the repository associated with this review. It also helps me keep track of open/closed issues that remain.

Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@kbarnhart) if you have any questions/concerns.

daniellivingston commented 4 years ago

@anowacki Great package. Does what it claims and does it very well and very elegantly. A minor nitpick and probably a personal taste issue; feel free to ignore.

You write that "[d]ocumentation is available at a dedicated website" without explicitly spelling out the URL. I understand the URL is relatively long (https://anowacki.github.io/SeisModels.jl/stable/), and that embedded hyperlinks mean that spelling out the full address is redundant, but I would still like to see the full path in the document. However, this is a minor thing and perhaps up to personal taste, so I will approve this paper regardless. Just something to think about.

daniellivingston commented 4 years ago

Also, in the documentation (https://anowacki.github.io/SeisModels.jl/stable/#How-to-install-1), one finds this:

How to install

SeisModels.jl can be added to your Julia environment like so:

julia> import Pkg; pkg"add https://github.com/anowacki/SeisModels.jl"

This should be changed to match your repo README:

SeisModels.jl can be added to your Julia install like so:

julia> import Pkg; Pkg.add("SeisModels")
daniellivingston commented 4 years ago

One of the review guidelines states:

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

Insofar as I can tell, discussions of third-party contributions are limited to this line in your README:

Currently, only three kinds of one-dimensional models are supported, but all model parameterisations and models are acceptable for inclusion. Contributions are welcome.

You may want to add a CONTRIBUTION.md document to the repo root, or just a small paragraph in the README expanding on your contribution process. You can find an over-the-top example of this here: https://reactjs.org/docs/how-to-contribute.html

anowacki commented 4 years ago

I have set up a branch joss-review in which I propose to make the changes before merging them in finally.

@daniellivingston—thank you very much for these helpful suggestions.

Docs website URL

You write that "[d]ocumentation is available at a dedicated website" without explicitly spelling out the URL. […] I would still like to see the full path in the document.

I'm very happy to go along with this and I have updated the manuscript accordingly.

Contribution guidelines

You may want to add a CONTRIBUTION.md document to the repo root, or just a small paragraph in the README expanding on your contribution process.

Thank you for helping me realise this—I had not quite understood the guidelines here, but do now on re-reading them.

I have added a CONTRIBUTING.md file (this filename seems much more common, at least on GirHub and Gitlab repos) to the repo root which hopefully explains things in appropriate detail.

Docs installation instructions

This should be changed to match your repo README:

Thank you for spotting this. I did update the docs a little while ago (in 3c9826ac), but it transpires that the docs stopped building after an earlier commit. I can see that a number of new functions are in fact missing. I will have to investigate this further to work out what has happened.

anowacki commented 4 years ago

@whedon generate pdf from branch joss-review

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-review. Reticulating splines etc...
whedon commented 4 years ago

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

anowacki commented 4 years ago

Docs installation instructions

This should be changed to match your repo README:

Thank you for spotting this. I did update the docs a little while ago (in 3c9826ac), but it transpires that the docs stopped building after an earlier commit. I can see that a number of new functions are in fact missing. I will have to investigate this further to work out what has happened.

Docs are now back up. (The issue was with Travis configuration.)

kbarnhart commented 4 years ago

Thanks to @daniellivingston for completing your review and for @anowacki for addressing the raised issues. 🎉

@fhorrobin and @joa-quim please work to complete your reviews in the next week or so (or provide a time estimate here).

Thanks all for participating in the JOSS review process.

joa-quim commented 4 years ago

Hi @kbarnhart, any particular action that I take in order to be able to start checking my boxes? Tried but apparently I have no permission to do so.

kbarnhart commented 4 years ago

@whedon add @joa-quim as reviewer

whedon commented 4 years ago

OK, @joa-quim is now a reviewer

kbarnhart commented 4 years ago

@joa-quim try now.

joa-quim commented 4 years ago

Sorry, not yet. To be clear, I should be able to check the boxes at the beginning of this page, right?

kbarnhart commented 4 years ago

Yup. You should be able to check the checkboxes under "Review checklist for @joa-quim". I'm not entirely certain how to fix this, so I'm tagging @arfon who should be able to provide guidance.

joa-quim commented 4 years ago

Thanks. And I'll complete the review this week (I've already installed the package and run some examples).

kbarnhart commented 4 years ago

@joa-quim I wanted to check in and see if you were able to check the boxes or if they are still not editable for you.

joa-quim commented 4 years ago

Nope, still blocked to me. And what about you @fhorrobin, can you check them?

kbarnhart commented 4 years ago

@arfon can you see anything about this issue that explains why @joa-quim can't check the boxes? I used whedon to assign him and he is now listed under assigned people. Thanks!

daniellivingston commented 4 years ago

@joa-quim If you want, you can make a comment with the above checklist and mark off what you've done. Since I have editing permissions on the top-level comment, I can edit the check boxes as a proxy for you.

joa-quim commented 4 years ago

Ok, I'll try to do that tomorrow afternoon/night.

arfon commented 4 years ago

@joa-quim - you need to accept the invite to the repository here: https://github.com/openjournals/joss-reviews/invitations to be able to edit the check boxes (it should be at the top of the page). When you click the 'accept', please make sure you're logged in with your @joa-quim account.

joa-quim commented 4 years ago

Hmm, something strange is still on. When I try to accept the invitation I get a Ooops 500. Looks like something went wrong! error from Github.

Anyway, my comment on this paper is that it is elegantly written and does what it describes,

I would only suggest that the installing instructions be changed to the shorter (and working)

]add SeisModels

instead of the longer version on the Github page

import Pkg; Pkg.add("SeisModels")

or the even longer in the documentation

import Pkg; pkg"add https://github.com/anowacki/SeisModels.jl"

Given that I'm still not able to check the boxes above, I reproduced the check boxes list bellow with the corresponding checks. Hope this is good enough as a workaround.

--------------------

kbarnhart commented 4 years ago

Thanks for your review @joa-quim! Sorry about the issues with check box permissions. I think your workaround is just fine.

I'll let @anowacki consider/address the small change you recommend in the main repository.

@fhorrobin would it be possible to get an update on when you expect to complete your review?

anowacki commented 4 years ago

Thanks, @joa-quim for the comments!

I would only suggest that the installing instructions be changed to the shorter (and working) ]add SeisModels instead of the longer version on the Github page import Pkg; Pkg.add("SeisModels")

My motivation for the latter command is that new Julia users can simply copy and paste the full command into a fresh REPL session without needing to worry about pkg mode, but I'd be happy to change this is you feel it is definitely less friendly.

or the even longer in the documentation import Pkg; pkg"add https://github.com/anowacki/SeisModels.jl"

When the package wasn't registered, this was needed, but no longer as I'm sure you realise. This part of the docs is a leftover from before I realised the docs weren't updating and the latest docs do not add using the GirHub URL.

joa-quim commented 4 years ago

@anowacki This was just my opinion on what looks more friendly to new users. More experimented users should know that already. But notice that one can do a copy past of ]add SeisModels string into a fresh REPL and it works fine. I mean, the REPL passes automatically into the pkg mode.

anowacki commented 4 years ago

But notice that one can do a copy past of ]add SeisModels string into a fresh REPL and it works fine. I mean, the REPL passes automatically into the pkg mode.

This doesn't actually work for me.

joa-quim commented 4 years ago

!! I'm on Windows and I was assuming that it would work in other OSs as well. Anyway, this is minor point. Do as you feel is better.

anowacki commented 4 years ago

Thanks for you suggestion, @joa-quim—just for the avoidance of doubt in the review process, I'm stating that I have decided to keep the docs as they are in this regard.

kbarnhart commented 4 years ago

@anowacki Thanks for noting your response @joa-quim's comment here for posterity.

@fhorrobin would it be possible to get an update on when you expect to complete your review?

@joa-quim and @daniellivingston thank you for completing your reviews.

A quick additional note. I accidentally turned on the JOSS out of office responder and have not figured out how to turn it off... So if you @ me, you will see an auto response that says I'm away until March 20th. This is not true, I'm not away... 😄

kbarnhart commented 4 years ago

Thanks to @daniellivingston and @joa-quim for completing your reviews.

@fhorrobin a friendly reminder to provide an update on when you expect to complete your review? If I don't hear back in the next day or two, I will likely decide to move forward with the review process (as JOSS only requires two reviews).

kbarnhart commented 4 years ago

I'm going to move this review forward.

Now that the two reviews have been completed, @daniellivingston and @joa-quim could you please reply and confirm your review recommendation.

At that point @anowacki could you please point me towards the current version of the paper (it is currently on a branch I think). I will provide some final, likely minor, editorial comments on the paper.

Once those are merged into the main repository branch, we will move to the final stages of the JOSS process (double checking reference DOIs, final paper compilation, setting archive, and version). @anowacki I'll ping you for what I need when we get there.

@fhorrobin thanks for being willing to undertake a review. It seems like things didn't work out with your schedule, no worries. I hope you'll consider being part of a JOSS review in the future. I'm going to remove you from this review now. Feel free to follow up with me here or via email if you'd like.

kbarnhart commented 4 years ago

@whedon remove @fhorrobin as reviewer

whedon commented 4 years ago

OK, @fhorrobin is no longer a reviewer

joa-quim commented 4 years ago

Hi, Yes, I confirm my recommendation for accepting this paper.

kbarnhart commented 4 years ago

@daniellivingston can I ping you to confirm your recommendation for this publication.

daniellivingston commented 4 years ago

@kbarnhart Yes, I confirm my recommendation for accepting this publication.

kbarnhart commented 4 years ago

Thanks @daniellivingston

kbarnhart commented 4 years ago

@whedon generate pdf from branch joss-review

@anowacki is this the most up to date version of the publication? I may have a couple of minor editorial comments on the paper---just want to make sure I'm looking at the right thing.

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-review
. Reticulating splines etc...
whedon commented 4 years ago

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

error: pathspec 'joss-review ' did not match any file(s) known to git. error: pathspec 'joss-review ' did not match any file(s) known to git. pandoc-citeproc: reference joa-quim not found Error producing PDF. ! TeX capacity exceeded, sorry [input stack size=5000]. \reserved@a ->\def \reserved@a *{\let \@xs@assign \@xs@expand@and@detokenize... l.330 }

Looks like we failed to compile the PDF

kbarnhart commented 4 years ago

@whedon generate pdf from branch joss-review

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-review. Reticulating splines etc...
whedon commented 4 years ago

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

pandoc-citeproc: reference joa-quim not found Error producing PDF. ! TeX capacity exceeded, sorry [input stack size=5000]. \reserved@a ->\def \reserved@a *{\let \@xs@assign \@xs@expand@and@detokenize... l.330 }

Looks like we failed to compile the PDF

arfon commented 4 years ago

@whedon generate pdf from branch joss-review

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-review. Reticulating splines etc...