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]: eSCAPE: parallel global-scale landscape evolution model #964

Closed
whedon opened this Issue Sep 18, 2018 · 31 comments

Comments

Projects
None yet
6 participants
@whedon
Collaborator

whedon commented Sep 18, 2018

Submitting author: @tristan-salles (Tristan Salles)
Repository: https://github.com/Geodels/eSCAPE
Version: v1.0.0
Editor: @kthyng
Reviewer: @sgrieve, @kbarnhart
Archive: 10.5281/zenodo.1419844

Status

status

Status badge code:

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

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

@sgrieve & @kbarnhart, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @kthyng know.

Please try and complete your review in the next two weeks

Review checklist for @sgrieve

Conflict of interest

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 (v1.0.0)?
  • Authorship: Has the submitting author (@tristan-salles) 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)?

Review checklist for @kbarnhart

Conflict of interest

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 (v1.0.0)?
  • Authorship: Has the submitting author (@tristan-salles) 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.

Show comment
Hide comment
@whedon

whedon Sep 18, 2018

Collaborator

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @sgrieve 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 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
Collaborator

whedon commented Sep 18, 2018

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @sgrieve 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 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
@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 18, 2018

Collaborator
Attempting PDF compilation. Reticulating splines etc...
Collaborator

whedon commented Sep 18, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 18, 2018

Collaborator

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

/app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in block in find': No such file or directory (Errno::ENOENT) from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in collect!'
from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/lib/whedon/processor.rb:57:in find_paper_paths'
from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:32:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:in run'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:in dispatch'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:99:in <top (required)>'
from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

Collaborator

whedon commented Sep 18, 2018

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

/app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in block in find': No such file or directory (Errno::ENOENT) from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in collect!'
from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/lib/whedon/processor.rb:57:in find_paper_paths'
from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:32:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:in run'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:in dispatch'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:99:in <top (required)>'
from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Sep 18, 2018

@whedon generate pdf

kthyng commented Sep 18, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 18, 2018

Collaborator
Attempting PDF compilation. Reticulating splines etc...
Collaborator

whedon commented Sep 18, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 18, 2018

Collaborator

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

/app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in block in find': No such file or directory (Errno::ENOENT) from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in collect!'
from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/lib/whedon/processor.rb:57:in find_paper_paths'
from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:32:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:in run'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:in dispatch'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:99:in <top (required)>'
from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

Collaborator

whedon commented Sep 18, 2018

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

/app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in block in find': No such file or directory (Errno::ENOENT) from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in collect!'
from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/lib/whedon/processor.rb:57:in find_paper_paths'
from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:32:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:in run'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:in dispatch'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-2caaf5cba519/bin/whedon:99:in <top (required)>'
from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Sep 18, 2018

ok @sgrieve and @kbarnhart I have this ready for you now. Let me know if you have any questions.

@arfon Do you have any guidance for how I can determine why the pdf is not compiling?

kthyng commented Sep 18, 2018

ok @sgrieve and @kbarnhart I have this ready for you now. Let me know if you have any questions.

@arfon Do you have any guidance for how I can determine why the pdf is not compiling?

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 18, 2018

Member

@whedon generate pdf

Member

arfon commented Sep 18, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 18, 2018

Collaborator
Attempting PDF compilation. Reticulating splines etc...
Collaborator

whedon commented Sep 18, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon
Collaborator

whedon commented Sep 18, 2018

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Sep 18, 2018

@arfon You must have the magic touch.

kthyng commented Sep 18, 2018

@arfon You must have the magic touch.

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 19, 2018

Member

@kthyng - sometimes @whedon just needs a little encouragement :-)

Member

arfon commented Sep 19, 2018

@kthyng - sometimes @whedon just needs a little encouragement :-)

@kbarnhart

This comment has been minimized.

Show comment
Hide comment
@kbarnhart

kbarnhart Sep 24, 2018

Collaborator

Since I've just loaded on a bunch of issues about documentation, testing, and installation, I thought I'd also make a note here that I think the core functionality of eSCAPE is great, the tutorials are very nicely done, and the docker image works as expected.

Collaborator

kbarnhart commented Sep 24, 2018

Since I've just loaded on a bunch of issues about documentation, testing, and installation, I thought I'd also make a note here that I think the core functionality of eSCAPE is great, the tutorials are very nicely done, and the docker image works as expected.

@sgrieve

This comment has been minimized.

Show comment
Hide comment
@sgrieve

sgrieve Sep 26, 2018

Collaborator

I would like to thank the author for this contribution, it has been a very enjoyable experience to explore this software and I am pleased to recommend publication, pending the minor issues highlighted below being resolved.

Version

I noticed that there is a version 1.0.1 on GitHub, but the submission corresponds to version 1.0.0. It would be good to clarify this, so that the current stable version is the one that is reviewed.

Installation

In the first instance I explored the code using Docker. Everything worked out of the box, and running the test notebook yielded the correct results with no surprises or problems. I would however, suggest that a little more detail in the main readme.md file on how to use Kitematic and Docker may be useful, as I imagine that this is the most likely method of install for new users, and those not familiar with Docker may get held up on this step.

One issue I noticed was that there is no requirements.txt file at the base of the repo, containing all of the python dependencies and their version requirements as listed on the HPC install page on the wiki, this would save people a bit of hassle during the HPC install process.

Functionality

Working my way through the extensive tutorials and examples showed me the range of applications for eSCAPE, and I am happy that the functional claims made in the documentation and the paper are valid. More broadly I believe that eSCAPE will be a valuable tool for researchers across the geosciences, and that it is providing a novel solution to the common problem of scale in the modeling of surface processes.

Performance

I have been unable to run this software on an HPC system (due to my current lack of access to one) so have only been able to explore the performance of eSCAPE on my MacBook. Where performance claims have been made, they have been in line with my experience of running eSCAPE. As I have tested larger and larger model runs, I have been pleasantly surprised by the compute times and believe that the performance of this model is in line with other similar model's performance, if not exceeding them.

Documentation

Going through the documentation I am impressed at the detail and quality of what has been produced. Most of the issues I would have flagged have already been resolved following the issues created by @kbarnhart.

One further issue I noticed was that in the homepage of the API documentation, there is a table listing submodules (mesher, flow, pit, tools). This table has no associated information about what these submodules are/what they do. Similarly, on the contents pane of the API docs there are links to these submodules (eSCAPE.mesher, eSCAPE.flow, eSCAPE.pit, eSCAPE.tools), but these links do not contain any information. I realize that Sphinx can be a pain for stuff like this, but if possible it would be good to either populate these pages if there is relevant information to go into them, or remove the links to avoid confusion.

Some other minor issues:

  • The About section of the wiki just contains some images, and no information.
  • The Contact section of the wiki links back to the wiki homepage, and provides no contact information
  • In the runModel notebook for the Australia tutorial, the ParaView command Warp by Scalar is called Wrap by Scalar. (I think this issue might be repeated in some of the other notebooks.)

Tests

The test notebook that is provided works well to ensure that the code is running as expected, and provides detailed information to users to begin debugging if things do not go as expected. In an ideal world there would be some kind of CI set up to run automated tests, but I appreciate the difficulties in doing this. I know that some UK research councils have a national Jenkins instance which supports automated testing and I don't know if that is something you can explore with your funders?

Paper

The paper clearly outlines the motivation for the development of eSCAPE, and the key theoretical underpinnings of the work. It reads clearly and follows the general pattern for JOSS papers.

Overall I am delighted to have had the opportunity to review this paper, and have been thoroughly impressed by the overall quality of the software, documentation and tutorials. I hope to continue using the software in the future! 🚀

Collaborator

sgrieve commented Sep 26, 2018

I would like to thank the author for this contribution, it has been a very enjoyable experience to explore this software and I am pleased to recommend publication, pending the minor issues highlighted below being resolved.

Version

I noticed that there is a version 1.0.1 on GitHub, but the submission corresponds to version 1.0.0. It would be good to clarify this, so that the current stable version is the one that is reviewed.

Installation

In the first instance I explored the code using Docker. Everything worked out of the box, and running the test notebook yielded the correct results with no surprises or problems. I would however, suggest that a little more detail in the main readme.md file on how to use Kitematic and Docker may be useful, as I imagine that this is the most likely method of install for new users, and those not familiar with Docker may get held up on this step.

One issue I noticed was that there is no requirements.txt file at the base of the repo, containing all of the python dependencies and their version requirements as listed on the HPC install page on the wiki, this would save people a bit of hassle during the HPC install process.

Functionality

Working my way through the extensive tutorials and examples showed me the range of applications for eSCAPE, and I am happy that the functional claims made in the documentation and the paper are valid. More broadly I believe that eSCAPE will be a valuable tool for researchers across the geosciences, and that it is providing a novel solution to the common problem of scale in the modeling of surface processes.

Performance

I have been unable to run this software on an HPC system (due to my current lack of access to one) so have only been able to explore the performance of eSCAPE on my MacBook. Where performance claims have been made, they have been in line with my experience of running eSCAPE. As I have tested larger and larger model runs, I have been pleasantly surprised by the compute times and believe that the performance of this model is in line with other similar model's performance, if not exceeding them.

Documentation

Going through the documentation I am impressed at the detail and quality of what has been produced. Most of the issues I would have flagged have already been resolved following the issues created by @kbarnhart.

One further issue I noticed was that in the homepage of the API documentation, there is a table listing submodules (mesher, flow, pit, tools). This table has no associated information about what these submodules are/what they do. Similarly, on the contents pane of the API docs there are links to these submodules (eSCAPE.mesher, eSCAPE.flow, eSCAPE.pit, eSCAPE.tools), but these links do not contain any information. I realize that Sphinx can be a pain for stuff like this, but if possible it would be good to either populate these pages if there is relevant information to go into them, or remove the links to avoid confusion.

Some other minor issues:

  • The About section of the wiki just contains some images, and no information.
  • The Contact section of the wiki links back to the wiki homepage, and provides no contact information
  • In the runModel notebook for the Australia tutorial, the ParaView command Warp by Scalar is called Wrap by Scalar. (I think this issue might be repeated in some of the other notebooks.)

Tests

The test notebook that is provided works well to ensure that the code is running as expected, and provides detailed information to users to begin debugging if things do not go as expected. In an ideal world there would be some kind of CI set up to run automated tests, but I appreciate the difficulties in doing this. I know that some UK research councils have a national Jenkins instance which supports automated testing and I don't know if that is something you can explore with your funders?

Paper

The paper clearly outlines the motivation for the development of eSCAPE, and the key theoretical underpinnings of the work. It reads clearly and follows the general pattern for JOSS papers.

Overall I am delighted to have had the opportunity to review this paper, and have been thoroughly impressed by the overall quality of the software, documentation and tutorials. I hope to continue using the software in the future! 🚀

@kbarnhart

This comment has been minimized.

Show comment
Hide comment
@kbarnhart

kbarnhart Sep 26, 2018

Collaborator

I thought I'd give a quick update on the progress of my review. As noted by @sgrieve I've made most of my comments in the form of issues. @tristan-salles has dealt with most of these impressively fast. This has been very helpful as I was able to attempt HPC installation based on the revised HPC instructions instead of the original ones.

I am testing the HPC installation instructions so between the two reviewers we will have tested all installation options. As of yesterday evening am hung up on a step related to the petsc4py installation done by my institutions Research Computing staff. @tristan-salles and I have been iterating on improvements to the HPC installation instructions on eSCAPE Issue #3. I don't expect that resolving my HPC issues will take too long. Once I've been able to confirm HPC install I'll be able finalize my review.

I anticipate that I will recommend acceptance with minor revisions (many of which @tristan-salles has already completed).

Collaborator

kbarnhart commented Sep 26, 2018

I thought I'd give a quick update on the progress of my review. As noted by @sgrieve I've made most of my comments in the form of issues. @tristan-salles has dealt with most of these impressively fast. This has been very helpful as I was able to attempt HPC installation based on the revised HPC instructions instead of the original ones.

I am testing the HPC installation instructions so between the two reviewers we will have tested all installation options. As of yesterday evening am hung up on a step related to the petsc4py installation done by my institutions Research Computing staff. @tristan-salles and I have been iterating on improvements to the HPC installation instructions on eSCAPE Issue #3. I don't expect that resolving my HPC issues will take too long. Once I've been able to confirm HPC install I'll be able finalize my review.

I anticipate that I will recommend acceptance with minor revisions (many of which @tristan-salles has already completed).

@tristan-salles

This comment has been minimized.

Show comment
Hide comment
@tristan-salles

tristan-salles Sep 27, 2018

Collaborator

A massive thank you to @kbarnhart and @sgrieve for their quick and super valuable inputs, comments & feedbacks!

Comments from @sgrieve

I think I have pretty much addressed all the comments. In details I have:

  • fixed the version to 1.0.0 which is now consistent with the one from the paper
  • added a requirements.txt file
  • updated the wiki with a step by step guide in regards to installing and using Docker and Kitematic (see)
  • updated the API to populated the submodule table and removing unnecessary submodules links (see)
  • updated the About section of the wiki
  • updated the Contact section of the wiki
  • fixed the typo in the demo python notebooks (warp by scalar to wrap by scalar)

Regarding the testing issue, I have added a test installation example in the eSCAPE-demo repository that provides two comparisons related to:

  • the expected values that you should obtained if your installation is successful
  • runtime for both serial and parallel simulations
    This test should take less than 1 minute. An example on how to visualise eSCAPE outputs in Paraview is also provided.

Comments from @kbarnhart

I think I have answered most of the issues and comments there (see the history of closed issues above). There is still a remaining open one: Improve eSCAPE HPCC installation instructions Geodels/eSCAPE#3.

Collaborator

tristan-salles commented Sep 27, 2018

A massive thank you to @kbarnhart and @sgrieve for their quick and super valuable inputs, comments & feedbacks!

Comments from @sgrieve

I think I have pretty much addressed all the comments. In details I have:

  • fixed the version to 1.0.0 which is now consistent with the one from the paper
  • added a requirements.txt file
  • updated the wiki with a step by step guide in regards to installing and using Docker and Kitematic (see)
  • updated the API to populated the submodule table and removing unnecessary submodules links (see)
  • updated the About section of the wiki
  • updated the Contact section of the wiki
  • fixed the typo in the demo python notebooks (warp by scalar to wrap by scalar)

Regarding the testing issue, I have added a test installation example in the eSCAPE-demo repository that provides two comparisons related to:

  • the expected values that you should obtained if your installation is successful
  • runtime for both serial and parallel simulations
    This test should take less than 1 minute. An example on how to visualise eSCAPE outputs in Paraview is also provided.

Comments from @kbarnhart

I think I have answered most of the issues and comments there (see the history of closed issues above). There is still a remaining open one: Improve eSCAPE HPCC installation instructions Geodels/eSCAPE#3.

@sgrieve

This comment has been minimized.

Show comment
Hide comment
@sgrieve

sgrieve Sep 27, 2018

Collaborator

Thanks for responding to my comments so quickly @tristan-salles!

@kthyng I'm satisfied that all of my comments have been addressed, so pending the resolution of the HPC install issues that @kbarnhart has raised I'm happy to formally accept!

Collaborator

sgrieve commented Sep 27, 2018

Thanks for responding to my comments so quickly @tristan-salles!

@kthyng I'm satisfied that all of my comments have been addressed, so pending the resolution of the HPC install issues that @kbarnhart has raised I'm happy to formally accept!

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Oct 1, 2018

I see that @sgrieve is ready to accept, and I see the issue referenced above that is in progress on @kbarnhart's end. Thanks for the great progress and work, @tristan-salles, @sgrieve, and @kbarnhart!

kthyng commented Oct 1, 2018

I see that @sgrieve is ready to accept, and I see the issue referenced above that is in progress on @kbarnhart's end. Thanks for the great progress and work, @tristan-salles, @sgrieve, and @kbarnhart!

@kbarnhart

This comment has been minimized.

Show comment
Hide comment
@kbarnhart

kbarnhart Oct 2, 2018

Collaborator

Alright. I'm completing my review now @kthyng .

Barnhart eSCAPE review

I'm happy to recommend acceptance contingent on minor revision.

This is an excellent contribution and I look forward to having applications that
require using it.

My primary concern is with HPC installation instructions. As I expect that many
major research-grade applications (and/or coupling with additional models) will
require a user to compile the code, I think that ironing these instructions out
is important.

Explanation for few checkboxes I didn't check.

I don't think any of these things should prevent this paper from moving forward.

I didn't check 'Installation: Does installation proceed as outlined in the
documentation?' as I was not able to surmount some of my HPC platform issues and
thus I never successfully installed this on an HPC. Ultimately the issue seems
to be related to the details of the Fortran compiler used in the backend of the
Intel compiler version that the staff at the University of Colorado Research
computing used to build PETSc.
@tristan-salles is in no way at fault in this and was very helpful throughout my attempt to compile. Based on my attempts I think
that the installation instructions are almost sufficient (see comments
below).

I also didn't check 'Installation instructions: Is there a clearly-stated
list of dependencies? Ideally these should be handled with an automated package
management solution.' I think that both the requirements.txt file and the HPC
installation instructions need to be revised (more details below).

I didn't check 'Automated tests: Are there automated tests or manual steps
described so that the function of the software can be verified?' because there
are no automated tests. However there are tests that meet the JOSS "good"
requirements
. I think there are reasonable reasons that there are no automated
tests (e.g. need a platform with PETSc, etc, to run the tests).

Recommended revision to to installation instructions.

I think that there are two items that relate to installation that need to be
addressed. The first relates to the requirements.txt and the second relates
to the HPC install instructions wiki page.

requirements.txt

I think that the requirements.txt file needs to be further revised in order
to include the minimum version numbers (where applicable). I also think that
having the fillit and meshplex package in the requirements.txt file is a
bit awkward as these are not packages that are distributed on standard package
distributors (conda, pip). Further the meshplex package is a modified version
forked by @tristan-salles. I see no problem with these as dependencies, but
putting these in a requirements.txt file means that a command like.

pip install -r requirements.txt

will fail because these packages are not findable.

HPC install instructions wiki

I think that the HPC installation page on the wiki
needs to be further revised to not be so specific to one HPC platform. I also
think that text related to the fortran compiler used is confusing.

Specificity regarding platform

Sections like this one on example dependencies
show loading specific modules like petsc built with gcc and open MPI. I would
recommend removing these lines and replacing with regular (not code block) text
that says something like:

Before you begin compiling eSCAPE and dependencies, make sure you have set up
your compute environment to include python, OpenMPI, a supported compiler, and
PETSc.

Now that a requirements .txt file exists, all the pip install commands can be
replaced by a single call

pip install -r requirements.txt

Combining all the code needed to install on the Artemis HPC at USyD is useful,
but perhaps should go on another page called "Example installation scripts used on specific HPC platforms"
Once I get my platform working I would happily submit my script there. This would help
draw the distinction for a user that there are general requirements, and that there are
specific examples that worked on a particular platform.

Fortran compiler

There are two places in the examples where the fortran compiler F90 is assigned
to something else (ifort in one place and gfortran in another). I will admit to
not knowing much about different fortran compilers (or different compilers in general...)
and thus these instructions are confusing to me. I might guess that I should be
using gfortran if I used gcc instead of the intel compiler, and ifort if I used
the intel compiler. I would recommend only writing

export F90=something

in one place on the HPC instructions and providing a bit of context as to why
someone should use gfortran or ifort in this situation.

Minor comments about the example

The example PBS script
assumes that a user has downloaded the eSCAPE-demo repository and dealt with all
of its requirements. It should be revised to include information about downloading
the demo repository. I also think that it would be more appropriate on another wiki
page.

Collaborator

kbarnhart commented Oct 2, 2018

Alright. I'm completing my review now @kthyng .

Barnhart eSCAPE review

I'm happy to recommend acceptance contingent on minor revision.

This is an excellent contribution and I look forward to having applications that
require using it.

My primary concern is with HPC installation instructions. As I expect that many
major research-grade applications (and/or coupling with additional models) will
require a user to compile the code, I think that ironing these instructions out
is important.

Explanation for few checkboxes I didn't check.

I don't think any of these things should prevent this paper from moving forward.

I didn't check 'Installation: Does installation proceed as outlined in the
documentation?' as I was not able to surmount some of my HPC platform issues and
thus I never successfully installed this on an HPC. Ultimately the issue seems
to be related to the details of the Fortran compiler used in the backend of the
Intel compiler version that the staff at the University of Colorado Research
computing used to build PETSc.
@tristan-salles is in no way at fault in this and was very helpful throughout my attempt to compile. Based on my attempts I think
that the installation instructions are almost sufficient (see comments
below).

I also didn't check 'Installation instructions: Is there a clearly-stated
list of dependencies? Ideally these should be handled with an automated package
management solution.' I think that both the requirements.txt file and the HPC
installation instructions need to be revised (more details below).

I didn't check 'Automated tests: Are there automated tests or manual steps
described so that the function of the software can be verified?' because there
are no automated tests. However there are tests that meet the JOSS "good"
requirements
. I think there are reasonable reasons that there are no automated
tests (e.g. need a platform with PETSc, etc, to run the tests).

Recommended revision to to installation instructions.

I think that there are two items that relate to installation that need to be
addressed. The first relates to the requirements.txt and the second relates
to the HPC install instructions wiki page.

requirements.txt

I think that the requirements.txt file needs to be further revised in order
to include the minimum version numbers (where applicable). I also think that
having the fillit and meshplex package in the requirements.txt file is a
bit awkward as these are not packages that are distributed on standard package
distributors (conda, pip). Further the meshplex package is a modified version
forked by @tristan-salles. I see no problem with these as dependencies, but
putting these in a requirements.txt file means that a command like.

pip install -r requirements.txt

will fail because these packages are not findable.

HPC install instructions wiki

I think that the HPC installation page on the wiki
needs to be further revised to not be so specific to one HPC platform. I also
think that text related to the fortran compiler used is confusing.

Specificity regarding platform

Sections like this one on example dependencies
show loading specific modules like petsc built with gcc and open MPI. I would
recommend removing these lines and replacing with regular (not code block) text
that says something like:

Before you begin compiling eSCAPE and dependencies, make sure you have set up
your compute environment to include python, OpenMPI, a supported compiler, and
PETSc.

Now that a requirements .txt file exists, all the pip install commands can be
replaced by a single call

pip install -r requirements.txt

Combining all the code needed to install on the Artemis HPC at USyD is useful,
but perhaps should go on another page called "Example installation scripts used on specific HPC platforms"
Once I get my platform working I would happily submit my script there. This would help
draw the distinction for a user that there are general requirements, and that there are
specific examples that worked on a particular platform.

Fortran compiler

There are two places in the examples where the fortran compiler F90 is assigned
to something else (ifort in one place and gfortran in another). I will admit to
not knowing much about different fortran compilers (or different compilers in general...)
and thus these instructions are confusing to me. I might guess that I should be
using gfortran if I used gcc instead of the intel compiler, and ifort if I used
the intel compiler. I would recommend only writing

export F90=something

in one place on the HPC instructions and providing a bit of context as to why
someone should use gfortran or ifort in this situation.

Minor comments about the example

The example PBS script
assumes that a user has downloaded the eSCAPE-demo repository and dealt with all
of its requirements. It should be revised to include information about downloading
the demo repository. I also think that it would be more appropriate on another wiki
page.

@tristan-salles

This comment has been minimized.

Show comment
Hide comment
@tristan-salles

tristan-salles Oct 3, 2018

Collaborator

Thanks @kbarnhart, this is again super useful. I have (I think) made all the changes required based on your comments/suggestions.

requirements.txt

I have updated the requirements.txt file with updated version number of each packages and removed custom packages dependencies.

Installation instructions

  • I have worked on the wiki pages and first moved the Docker page to the top, prior to the local and HPC installation as it is likely going to be the easiest way for people to start using the code.

Then I have created an additional page:

  • Local installation that provides a step by step guide for local installation. I have tried to keep it as generic as possible to remove any OS specific instructions.

After I reshaped the:

  • HPC installation page following your suggestions (e.g, not be so specific to one HPC platform, confusion with fortran compilers, removing PBS script...)

Finally I added more content to the Tutorials page, explaining how to use the provided examples and how to create the required input files. I also added to this page a section on how to run eSCAPE as well as the example of PBS script.

Collaborator

tristan-salles commented Oct 3, 2018

Thanks @kbarnhart, this is again super useful. I have (I think) made all the changes required based on your comments/suggestions.

requirements.txt

I have updated the requirements.txt file with updated version number of each packages and removed custom packages dependencies.

Installation instructions

  • I have worked on the wiki pages and first moved the Docker page to the top, prior to the local and HPC installation as it is likely going to be the easiest way for people to start using the code.

Then I have created an additional page:

  • Local installation that provides a step by step guide for local installation. I have tried to keep it as generic as possible to remove any OS specific instructions.

After I reshaped the:

  • HPC installation page following your suggestions (e.g, not be so specific to one HPC platform, confusion with fortran compilers, removing PBS script...)

Finally I added more content to the Tutorials page, explaining how to use the provided examples and how to create the required input files. I also added to this page a section on how to run eSCAPE as well as the example of PBS script.

@kbarnhart

This comment has been minimized.

Show comment
Hide comment
@kbarnhart

kbarnhart Oct 3, 2018

Collaborator

@kthyng a quick note that I've now closed all the issues I opened in the eSCAPE repository.

Collaborator

kbarnhart commented Oct 3, 2018

@kthyng a quick note that I've now closed all the issues I opened in the eSCAPE repository.

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Oct 4, 2018

@kbarnhart Has @tristan-salles completed or otherwise addressed the minor revisions you noted or are there any outstanding issues?

kthyng commented Oct 4, 2018

@kbarnhart Has @tristan-salles completed or otherwise addressed the minor revisions you noted or are there any outstanding issues?

@kbarnhart

This comment has been minimized.

Show comment
Hide comment
@kbarnhart

kbarnhart Oct 4, 2018

Collaborator
Collaborator

kbarnhart commented Oct 4, 2018

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Oct 8, 2018

Ok thanks @kbarnhart, just wanted to be sure.

@arfon This paper has been accepted by the reviewers (@kbarnhart and @sgrieve)!!!! Ready for publication!!

kthyng commented Oct 8, 2018

Ok thanks @kbarnhart, just wanted to be sure.

@arfon This paper has been accepted by the reviewers (@kbarnhart and @sgrieve)!!!! Ready for publication!!

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Oct 8, 2018

@tristan-salles Please create an archive (on Zenodo, figshare, or other) and post the archive DOI in this issue.

kthyng commented Oct 8, 2018

@tristan-salles Please create an archive (on Zenodo, figshare, or other) and post the archive DOI in this issue.

@kthyng kthyng added the accepted label Oct 8, 2018

@tristan-salles

This comment has been minimized.

Show comment
Hide comment
@tristan-salles

tristan-salles Oct 8, 2018

Collaborator

Hi @kthyng here is the zenodo archive:
DOI

DOI: 10.5281/zenodo.1419844

Collaborator

tristan-salles commented Oct 8, 2018

Hi @kthyng here is the zenodo archive:
DOI

DOI: 10.5281/zenodo.1419844

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng Oct 8, 2018

@whedon set 10.5281/zenodo.1419844 as archive

kthyng commented Oct 8, 2018

@whedon set 10.5281/zenodo.1419844 as archive

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Oct 8, 2018

Collaborator

OK. 10.5281/zenodo.1419844 is the archive.

Collaborator

whedon commented Oct 8, 2018

OK. 10.5281/zenodo.1419844 is the archive.

@kthyng

This comment has been minimized.

Show comment
Hide comment
@kthyng

kthyng commented Oct 8, 2018

Thanks @tristan-salles!

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 9, 2018

Member

@sgrieve, @kbarnhart - many thanks for your reviews here and to @kthyng for editing this submission

@tristan-salles - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00964 ⚡️ 🚀 💥

Member

arfon commented Oct 9, 2018

@sgrieve, @kbarnhart - many thanks for your reviews here and to @kthyng for editing this submission

@tristan-salles - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00964 ⚡️ 🚀 💥

@arfon arfon closed this Oct 9, 2018

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Oct 9, 2018

Collaborator

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

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.00964/status.svg)](https://doi.org/10.21105/joss.00964)

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

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.00964/status.svg
   :target: https://doi.org/10.21105/joss.00964

This is how it will look in your documentation:

DOI

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:

Collaborator

whedon commented Oct 9, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

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.00964/status.svg)](https://doi.org/10.21105/joss.00964)

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

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.00964/status.svg
   :target: https://doi.org/10.21105/joss.00964

This is how it will look in your documentation:

DOI

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:

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