search

openjournals / joss-reviews

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

[REVIEW]: PorousFlow: a multiphysics simulation code for coupled problems in porous media #2176

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @WilkAndy (Andy Wilkins) Repository: https://github.com/idaholab/moose Version: snapshot-20-10-27 Editor: @jedbrown Reviewer: @jbrezmorf, @rpodgorney Archive: 10.5281/zenodo.4071026

Status

status

Status badge code:

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

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

@jbrezmorf & @rpodgorney, 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 @jedbrown know.

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

Review checklist for @jbrezmorf

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @rpodgorney

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. @jbrezmorf, @rpodgorney 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

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

jedbrown commented 4 years ago

@jbrezmorf, @rpodgorney :wave: Welcome to JOSS and thanks for agreeing to review!

The comments from @whedon above outline the review process, which takes place in this thread (possibly with issues filed in the MOOSE repository, which is where PorousFlow resides). I'll be watching this thread if you have any questions.

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/2176 so that a link is created to this thread (and I can keep an eye on what is happening). 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 a month or so. Please let me know if 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 (@jedbrown) if you have any questions/concerns.

jbrezmorf commented 4 years ago

@jedbrown Hi Jed. Trying to proceed with the review I realized that my JOSS invitation has expired and I can not fill the checklist. Sorry, I didn't notice the invitation before.

arfon commented 4 years ago

@whedon re-invite @jbrezmorf as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@jbrezmorf please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

arfon commented 4 years ago

@jedbrown Hi Jed. Trying to proceed with the review I realized that my JOSS invitation has expired and I can not fill the checklist. Sorry, I didn't notice the invitation before.

☝️ You should be re-invited now.

jbrezmorf commented 4 years ago

Software: ======= no major issues, some tips fo improvements in the issue.

Summary: ======= well written, a typographic error in Acknowledgements: "Philipp Sch{”a}dle."

arfon commented 4 years ago

:wave: @rpodgorney - today we reopened JOSS for new submissions and are checking in on our existing reviews. Do you think you might be able to wrap up your review in the next 2-3 weeks?

rpodgorney commented 4 years ago

Sorry for letting this slip. All in all no issues at all, the code, tests, and documentation are al superb.

The paper is well written and contains just the right amount of details without becoming overly verbose.

rpodgorney commented 4 years ago

@arfon this is my first review for JOSS. I've looked through the code, documentation, and tests -- and feel it is OK to publish. Other than the checklist, is there anything else I need to do to complete/document my approval?

arfon commented 4 years ago

@arfon this is my first review for JOSS. I've looked through the code, documentation, and tests -- and feel it is OK to publish. Other than the checklist, is there anything else I need to do to complete/document my approval?

Nope, that's it. Over to @jedbrown to decide on next steps.

jedbrown commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

jedbrown commented 4 years ago

Sorry about my delay. This is looking great.

  1. There is a period missing after the Lime reference (end of paragraph).
  2. "simply solve simplistic" -> "only solve simplistic" or some other phrasing.
  3. Could you make any comments in the comparison with other software, perhaps in terms of capability, performance, and easy of use/extension?
  4. You're welcome to include a figure if you'd like.
  5. Would it make sense to write down the basic equations that are solved? Perhaps one or two lines for each of THMC just to get the structural gist across, or a figure representing the role and coupling. (It's sort of abrupt to launch directly into prior applications).
  6. Can you double-check capitalization in your references? Our toolchain uses pandoc-citeproc, which is based on apa.csl. Its semantics are closer to biblatex than bibtex, which is required to be precisely APA-conforming. Anyway, there are extra braces needed, e.g., {I}celand or {Iceland} are needed.
WilkAndy commented 4 years ago

Thank you @jedbrown for all those comments. We'll get onto them next week.

jedbrown commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1007/s12665-012-1546-x is OK
- 10.1007/s00366-006-0049-3 is OK
- 10.1080/22020586.2019.12073198 is OK

MISSING DOIs

- https://doi.org/10.1016/j.softx.2020.100430 may be missing for title: MOOSE: Enabling Massively Parallel Multiphysics Simulation
- https://doi.org/10.21203/rs.2.17644/v1 may be missing for title: Impact of Effective Normal Stress on Capillary Pressure in a Single Natural Fracture

INVALID DOIs

- None
WilkAndy commented 4 years ago

Hi @jedbrown . I've just submitted a PullRequest to MOOSE for the changes you request above. Perhaps you'd like to review the changes there? https://github.com/idaholab/moose/pull/15540

WilkAndy commented 4 years ago

The above changes have now been merged into the next branch of the MOOSE repo. Note the branch - at some stage we changed whedon's branch to joss_porous_flow , but that has not actually been used for all the editorial+reviewer changes. So the joss_porous_flow branch should not be used for our JOSS paper: instead use the next branch.

Thanks!

andy

jedbrown commented 4 years ago

@whedon generate pdf from branch next

whedon commented 4 years ago 
Attempting PDF compilation from custom branch next. Reticulating splines etc...
jedbrown commented 4 years ago

@whedon check references

whedon commented 4 years ago

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

sh: 1: cd: can't cd to tmp/2176 /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - tmp/2176 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-446a0298a33b/lib/whedon/processor.rb:61:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-446a0298a33b/bin/whedon:50: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-446a0298a33b/bin/whedon:119: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

'

jedbrown commented 4 years ago

@openjournals/dev I see the paper.md in the same place. Is this a Whedon issue?

arfon commented 4 years ago

@whedon generate pdf from branch next

whedon commented 4 years ago 
Attempting PDF compilation from custom branch next. Reticulating splines etc...
whedon commented 4 years ago

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

arfon commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1007/s12665-012-1546-x is OK
- 10.1007/s00366-006-0049-3 is OK
- 10.21203/rs.2.17644/v1 is OK
- 10.1080/22020586.2019.12073198 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.softx.2020.100430 is INVALID because of 'https://doi.org/' prefix
arfon commented 4 years ago

@jedbrown - not sure what was going on here. Seems to be working now.

jedbrown commented 4 years ago

Thanks @arfon

WilkAndy commented 4 years ago

I can fix the https://doi.org/ early next week

jedbrown commented 4 years ago

While you're at it, could you avoid repeating the repository URL (which already exists in the sidebar)? Also, it's a bit unusual to launch into a bullet list of applications after only one short sentence saying what the package does. I suppose I don't object to the present organization, but an EiC might.

I have a question outside the context of the review: is the global conservation exact (to solver tolerance) or only up to discretization error? The skeleton velocity term not being in divergence form looks suspicious to me.

WilkAndy commented 4 years ago

MOOSE issue to address the "https" and "repo URL" at https://github.com/idaholab/moose/issues/15591

WilkAndy commented 4 years ago

I've submitted a PullRequest to the MOOSE repo for these changes: https://github.com/idaholab/moose/pull/15592

WilkAndy commented 4 years ago

@whedon generate pdf from branch next

whedon commented 4 years ago 
Attempting PDF compilation from custom branch next
. Reticulating splines etc...
whedon commented 4 years ago

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

WilkAndy commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1007/s12665-012-1546-x is OK
- 10.1016/j.softx.2020.100430 is OK
- 10.1007/s00366-006-0049-3 is OK
- 10.21203/rs.2.17644/v1 is OK
- 10.1080/22020586.2019.12073198 is OK

MISSING DOIs

- None

INVALID DOIs

- None
WilkAndy commented 4 years ago

Hi @jedbrown

Good to publish?

andy

WilkAndy commented 4 years ago

Hi @jedbrown . Just wondering about the status of this JOSS paper. I think we've addressed all referee and editorial iteams and it's read to go. If not, please tell me what needs to be done.

jedbrown commented 4 years ago

Sorry about my delays. It's looking great.

Once you've addressed these, please:

I can then move forward with accepting the submission.

WilkAndy commented 4 years ago

Sounds good. I shall make those changes and archive the software soon. Idaho National Labs has been experiencing some sort of software/hardware/electrical problems for a while and consequently the MOOSE testing/merging/etc has been held up. So it might be a couple of weeks before these changes+archiving gets completed.

jedbrown commented 4 years ago

FWIW, these small changes to the paper do not need to appear in the archived version, but it should be a tagged release that includes any substantive changes resulting from the review. I see only one tag in the MOOSE repository; let us know if tagging a release (v1.0.1 or some such) is problematic.

kthyng commented 4 years ago

Hi all! I am the rotating editor in chief this week. I see it has been awhile since there has been activity on this submission, but it looks pretty much done. @jedbrown what steps are left before you'd like me to take over to finish publishing? @WilkAndy have you finished the items listed just above here?

WilkAndy commented 4 years ago

Hi @kthyng , I just got back from holidays and am addressing a backlog of TODO items. I have not implemented the items mentioned above by @jedbrown, but hope to do them this week. However, because of a disastrous computer crash at Idaho National Labs, the whole MOOSE project has been severely impacted so I don't expect the changes, tagging, etc, will be reviewed and merged within the week, so I think you can safely forget about it during your editor-in-chief week.

kthyng commented 4 years ago

@WilkAndy Got it! No problem, happy to hear there is a path forward. Good luck with the computer crash.

WilkAndy commented 4 years ago

@jedbrown 's final suggestions have been implemented and merged into the master branch

tag = https://github.com/idaholab/moose/releases/tag/snapshot-20-10-27 (ie, tag name is snapshot-20-10-27 )

https://zenodo.org/record/4071026#.X5kq-9sRXOQ

Metadata is correct

doi = 10.5281/zenodo.4071026