Joss-reviews: [REVIEW]: BATMAN: Statistical analysis for expensive computer codes made easy

Created on 11 Dec 2017  ·  51Comments  ·  Source: openjournals/joss-reviews

Submitting author: @tupui (Pamphile ROY)
Repository: https://gitlab.com/cerfacs/batman
Version: v1.7
Editor: @katyhuff
Reviewer: @mikeckennedy
Archive: 10.5281/zenodo.1163545

Status

status

Status badge code:

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

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

@mikeckennedy, 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 @katyhuff know.

Conflict of interest

Code of Conduct

General checks

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

Functionality

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

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

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

Most helpful comment

@scollis @mikeckennedy - many thanks for your reviews here and to @katyhuff for editing this submission ✨

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

All 51 comments

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @mikeckennedy it looks like you're currently assigned as the reviewer for 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
Attempting PDF compilation. Reticulating splines etc...
https://github.com/openjournals/joss-papers/blob/joss.00493/joss.00493/10.21105.joss.00493.pdf

@scollis please note that Joss isn't great at handling multiple reviewers at the moment. I still would love to have your review on this paper. The checklist above is for the other reviewer. Please use this one, below, for your review.

@scollis, 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 @katyhuff know.

Conflict of interest

Code of Conduct

General checks

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

Functionality

  • [x] 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?
  • [x] 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

  • [x] 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)?

Hey @katyhuff I'll be doing this over the Xmas break.. Hands full finishing a few things off.. Sorry for the delay.

Hi @katyhuff Ditto. It's been really hectic with end of year stuff for my business too. I'll try to get to it shortly.

Hi @katyhuff, how does it works for the version of the software? master was 1.7, now it’s 1.7.1 but we are updating regularly. So does it means that review I happening on 1.7 and that it is from this version that the article will be about?

Hi @katyhuff and @tupui, just wanted to let you know I'm making my way through this now. Got my year-end blockers out of the way. I probably won't finish it tonight but am getting close.

@tupui I do seem to be stuck trying to run the tests. Is there a good way for us to discuss? Should I just create an issue on Gitlab?

@tupui @katyhuff I made it a private issue here, feel free to make it public if you want.

https://gitlab.com/cerfacs/batman/issues/95

@mikeckennedy Thank you for taking the time to do this. I guess the issue tracker is a good place for this. I will make it public for transparency 😃

@tupui The version part is a bit tricky. Since many of the review-related fixes will be in minor versions post 1.7, we'll change the review version to a later one and have you create a zenodo or figshare item at that final version number that reflects the reviewed-and-accepted version of the software. So, the order of operations is: continue to update the software until the reviewers are satisfied, then we'll accept the paper into joss, and at that moment I'll ask you to update the paper metadata to reflect that updated version of the software (perhaps by then it will be 1.7.3 or something) and create a specific zenodo doi for that version as well. Your JOSS paper will be about version 1.7.3, in that example. Make sense? We'll walk through it as it progresses.

@katyhuff That makes sense to me. Thank you for the clarification.

Hey @katyhuff @tupui and @mikeckennedy Sorry for my obscene delay.. Ametsoc annual meeting is just winding down and I installed Batman today... Will work through the examples etc..

@scollis No worries. I you have any question. Also you can open an issue on the bug tracker, as with mikeckennedy I can add you as Guest on the project.

Review (to date):
-Repository is located as indicated by the link
-Licence is a CERFACS specific licence and is OSI approved.
-Version is 1.7.1 matching the version 1.7 as indicated in the submisison
-Authorship includes all contributors with greater that 10 commits. This seems reasonable.

@scollis Some info about the licence. CECILL licence are not a CERFACS thing but it was created by CEA, CNRS and Inria. The purpose is to have an OSI licence compatible with french law in case of lawsuit.

Thanks! very interesting!

Hey @tupui
Installation worked but I am trying to run a simple example..

I see here: https://cerfacs.gitlab.io/batman/tutorial/michalewicz.html you have an example using json to define the problem

I grabbed the gitlab repo and changed to test_cases/Michalewicz directory and ran
batman settings.json -qsu
I am attaching a (long ish) text file with the errors..

You do not make it too clear how to set up this example case

Hi @scollis, I see the problem. This is coming from the fact that the documentation corresponds to the latest commit on the develop branch. We do this because currently all our users are always up to date with the develop branch.

So the issue comes from the fact that we introduced a change in the settings syntax (we simplified it). With your current version, the settings in the respective folder in test_cases should be correct.

The current documentation for the source you have can be compiled using: python setup.py build_sphinx. Then open doc/_build/index.html.

Maybe we should have a warning to explain that the doc corresponds to the latest commit on develop. Otherwise I will see if we can afford going to read the doc to have doc versioning directly in the browser.

Yes.. but i installed using conda from conda forge.
So do I need to find the correct commit?

Perhaps you should cut a new conda-forge release for this submission as the first install method that users come across (which is very easy!) is the 'conda env create' one which is incompatible with the examples.

I am really looking for a "Hello world" example that a user could use to get started in order to verify the functionality as part of my review

@scollis, the version you installed using conda is 1.7.1. Indeed it does not have the latest change so I will do a 1.7.2 with the update on conda.

In the meantime, I have started to move the doc to read the doc so you can have the doc for your current version: http://batman.readthedocs.io/en/1.7.1-lucius/tutorial.html
[EDIT:] looks good except the API part, I still have to figure out how to have the code shown.

The case described in the tutorial is the same as the case in test_cases/Michalewicz so you do not have to manually recreate the settings.json. You can thus directly play with the settings from here.

Hope this helps. Let me know if there is any mistakes or something which is still unclear and I'll be happy to help.

Thanks!

Hi @scollis. I have done the read the docs documentation and the conda update to 1.7.2. So everything should be coherent now. The readme has been update accordingly to point to the new documentation. So at the bottom left of the documentation page, the user can select the version of the documentation.

http://batman.readthedocs.io

Thanks @tupui ! Fixing the CI on one of my packages today should be back to this soon

@scollis no problem, thanks! For info, pipeline is marked as failed but it is only due to the documentation that is not anymore on gitlab. I will change the general settings to clean this. Just to say that the package itself is green :)
[EDIT:] now back to green.

Thanks guys. I have basically finished my review. Do you happen to have the a working version of the tutorial code (basically a directory you can zip up rather than steps). I'm not 100% sure I got it installed correctly given the weird errors I got running the tests.

Hi @mikeckennedy, thanks for your work. Concerning the tutorial, here is a
zip. This is basically the same folder that you can find in the git repository under test_cases/Michalewicz. To launch batman just go to the folder and do batman settings.json. You can play with options of the CLI of course. If you have any question, I will be happy to help.

Thank you both. That plus switching to conda-forge on ubuntu (rather than straight conda on my mac) seemed to work. Got the review all done and everything looks great to me. Here are my notes, let me know if more than this + sign off is expected.

Review notes (no particular order of importance)

General observations

  • I think requirement.txt should be plural. Typically it's requirements.txtbut nothing major here.
  • One issue I did find was requirement.txt seemed good but was missing pylint whereas setup.py has it in tests_require. Just making sure that you wanted this. I thought this may be left out of requirements because you wanted to omit testing libraries. But you do have pytest-* in there so probably pylint belongs too.

The paper [pass]:

The JOSS paper (the PDF associated with this submission) should only include:

  • [check] A list of the authors of the software
  • [check]Author affiliations
  • [check]A short summary describing the high-level functionality of the software
  • [check]A list of key references including a link to the software archive

Software license [pass]

  • Version [conditional: expected 1.7, got 1.7.1-Lucius, but I was maybe too slow as well]

A statement of need [pass]

Documentation [pass]

A statement of need [pass]

Installation instructions [OK]

  • Could include things needed like Fortran compilers and OpenTURNS a little better.
  • I would make the conda install directions more prominant. I struggled to get this working on my mac, switched to ubuntu 17.10 and used the conda forge version and it was smooth then.

Example usage [pass]

API documentation [pass]

Contributing [pass]

https://cerfacs.gitlab.io/batman/contributing_link.html

Tests [pass]

Ultimately just checked in on the CI/CD running via Gitlab

Functionality [pass]

Conclusion

Nice project, glad to give it a positive review.

Hi @mikeckennedy, thank you very much for your review.

Indeed will update the requirements file. I wanted to avoid testing packages as you pointed out but pytest-runner is required by setup.py that is why I had to keep this one. But I agree that this is not coherent not to have pylint. Will add it.

Regarding the issue on mac os. Do you have more info except the one you provided on the issue tracker? Or is it something else? Along with the doc update, I pushed 1.7.2 ton conda-forge.

I will improve the installation instruction with current integration of a PR about IO. This PR will remove all fortran code.

Thanks again for your time, really appreciate it!

Thanks both. @mikeckennedy , thank you for this review. @tupui, please let me know in this issue once these items have been completed and we'll move on to next steps!

@scollis - do you have any final review comments for @tupui? It looks like you did a good first round review, thanks! I just wanted to circle back and see if there were any remaining items you wanted to note.

@katyhuff All good! @mikeckennedy covered all the points I have not yet.. Its good to go on my end!

Nice software and one of the coolest logos (the bat) I have seen :)

@scollis Thank you for your review and for your time on this. I will send the kind words about the logos ;). @katyhuff We are currently working on this. It should be done by the end of the day. Then, will do a 1.7.3 to fix the version for the paper and will let you know here. Do I need to update something in the paper prior to the release (except the version number I guess), like dates?

@katyhuff merged on develop, only waiting for next instruction before making the release to 1.7.3. I have already prepared the PR for this.

@tupui You're welcome. Thank you.

@tupui Great. I can't see your PR,
image

In any case, this looks good to me! I believe it can be accepted! At this point could you make an updated archive of the reviewed version of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

Hi @katyhuff, sorry for the PR, I cannot find you on gitlab to add you to the project so you can view it. If you give me your pseudo I can if you want.

Otherwise, I merged it and I have done the archive on Zenodo: https://zenodo.org/record/1163545
DOI is: 10.5281/zenodo.1163545
I updated the conda-forge version as well. For info, I messed up the python 2 docker image, hence the build failing. I am recovering this as I write. But build is passing on the python 3 image so all is good :)

@tupui It's interesting that the repository is open but that the PRs aren't. Have you thought about making them open? That would be much more in line with the open source mentality here at JOSS -- particularly, there is a requirement that there be clear guidelines about how to contribute.

Sorry I did not check for this.. But I did go to the contributors page on gitlab and could see the number of commits from each contributor. I used this to confirm the Authorship was accurate

@katyhuff Indeed, this was just a settings issue from my part. Sorry for this I thing I fixed this now. I am new to gitlab.

Also, I fixed the pipeline by fixing the images as I said.

Is there anything else I can do?

JOSS: Making software better! Love it

@whedon set 10.5281/zenodo.1163545 as archive

OK. 10.5281/zenodo.1163545 is the archive.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...
https://github.com/openjournals/joss-papers/blob/joss.00493/joss.00493/10.21105.joss.00493.pdf

@arfon I think we're ready to accept this!

@scollis @mikeckennedy - many thanks for your reviews here and to @katyhuff for editing this submission ✨

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

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippet:

[![DOI](http://joss.theoj.org/papers/10.21105/joss.00493/status.svg)](https://doi.org/10.21105/joss.00493)

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 volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html

This is great! Thank you everyone for this work. I really enjoyed the publication process. This is so much transparent, fast and simple than traditional journal. Looking forward to do some review if needed :)

Was this page helpful?
0 / 5 - 0 ratings