search

openjournals / joss-reviews

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

[REVIEW]: Ising_OPV v4.0: Experimental Tomography Data Import, Interpretation, and Analysis #1072

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @MikeHeiber (Michael Heiber) Repository: https://github.com/MikeHeiber/Ising_OPV Version: v4.0.0 Editor: @katyhuff Reviewer: @myousefi2016, @stuartcampbell, @mdoucet Archive: 10.5281/zenodo.1710685

Status

status

Status badge code:

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

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

@myousefi2016 & @stuartcampbell & @mdoucet, 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.

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

Review checklist for @myousefi2016

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @stuartcampbell

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mdoucet

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @myousefi2016, 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
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

% Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 15 0 15 0 0 329 0 --:--:-- --:--:-- --:--:-- 333 pandoc-citeproc: reference pfannmoeller2013ees not found pandoc-citeproc: reference vanbavel2009nl not found pandoc-citeproc: reference pfannmoeller2013ees not found pandoc-citeproc: reference proudian2018arXiv not found pandoc-citeproc: reference heiber2018ising4 not found Error producing PDF. ! Missing $ inserted.

$ l.265 ...{Acknowledgments}\label{acknowledgments}} Looks like we failed to compile the PDF
MikeHeiber commented 5 years ago

@whedon generate pdf from branch joss-edits

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

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

% Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 15 0 15 0 0 320 0 --:--:-- --:--:-- --:--:-- 326 Error producing PDF. ! Missing $ inserted.

$ l.279 Looks like we failed to compile the PDF
MikeHeiber commented 5 years ago

@arfon Could you compile the again locally for me from the joss-edits branch? Still not sure why the compilation is failing.

arfon commented 5 years ago

@arfon Could you compile the again locally for me from the joss-edits branch? Still not sure why the compilation is failing.

Sure thing: 10.21105.joss.01072.pdf

myousefi2016 commented 5 years ago

@MikeHeiber I'm trying to run the MPI test but I'm getting this error:

[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from MPI_Test
[ RUN      ] MPI_Test.CalculateProbHistAvgTests
test/test_mpi.cpp:46: Failure
Expected equality of these values:
  6
  (int)prob.size()
    Which is: 3
[  FAILED  ] MPI_Test.CalculateProbHistAvgTests (0 ms)
[ RUN      ] MPI_Test.CalculateVectorAvgTests
test/test_mpi.cpp:68: Failure
Expected equality of these values:
  4.5
  data_avg[0]
    Which is: 0
test/test_mpi.cpp:69: Failure
Expected equality of these values:
  5.5
  data_avg[1]
    Which is: 1
test/test_mpi.cpp:70: Failure
Expected equality of these values:
  6.5
  data_avg[2]
    Which is: 2
[  FAILED  ] MPI_Test.CalculateVectorAvgTests (1 ms)
[ RUN      ] MPI_Test.CalculateVectorSumTests
test/test_mpi.cpp:83: Failure
Expected equality of these values:
  18.0
    Which is: 18
  data_sum[0]
    Which is: 0
test/test_mpi.cpp:84: Failure
Expected equality of these values:
  22.0
    Which is: 22
  data_sum[1]
    Which is: 1
test/test_mpi.cpp:85: Failure
Expected equality of these values:
  26.0
    Which is: 26
  data_sum[2]
    Which is: 2
test/test_mpi.cpp:94: Failure
Expected equality of these values:
  18
  data_sum2[0]
    Which is: 0
test/test_mpi.cpp:95: Failure
Expected equality of these values:
  22
  data_sum2[1]
    Which is: 1
test/test_mpi.cpp:96: Failure
Expected equality of these values:
  26
  data_sum2[2]
    Which is: 2
[  FAILED  ] MPI_Test.CalculateVectorSumTests (0 ms)
[ RUN      ] MPI_Test.GatherValuesTests
[       OK ] MPI_Test.GatherValuesTests (0 ms)
[ RUN      ] MPI_Test.GatherVectorsTests
test/test_mpi.cpp:151: Failure
Expected equality of these values:
  12
  (int)data_all.size()
    Which is: 3
test/test_mpi.cpp:162: Failure
Expected equality of these values:
  12
  (int)data_all2.size()
    Which is: 3
[  FAILED  ] MPI_Test.GatherVectorsTests (0 ms)
[----------] 5 tests from MPI_Test (1 ms total)

[----------] Global test environment tear-down
[==========] 5 tests from 1 test case ran. (1 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 4 tests, listed below:
[  FAILED  ] MPI_Test.CalculateProbHistAvgTests
5 FAILED TESTS

By the way I baked your code by using OpenMPI/1.10.3 and gcc/6.3.0 and I'm using InfiniBand in our cluster. Any idea or suggestion?

MikeHeiber commented 5 years ago

@myousefi2016 Can you file an issue on the Ising_OPV repo with this info and also include the command that you used when running the test. I will take a look and see what the problem is.

katyhuff commented 5 years ago

(for provenance, that issue is https://github.com/MikeHeiber/Ising_OPV/issues/10)

myousefi2016 commented 5 years ago

@katyhuff I'm pretty much done with reviewing this software. @MikeHeiber Really interesting application to generate 3D microstructures. Good job Mike!

mdoucet commented 5 years ago

I've completed my review. Looks good!

katyhuff commented 5 years ago

@myousefi2016 and @mdoucet Thanks both for your prompt and thorough reviews! @MikeHeiber thank you for engaging promptly with reviewer comments.

We'll give @stuartcampbell a chance to complete functionality and documentation checks, and then we'll soon have a full slate of reviews.

stuartcampbell commented 5 years ago

Should be finished later today

MikeHeiber commented 5 years ago

@myousefi2016 The issue you filed due to incorrect testing instructions in the README has been fixed in the latest version on the joss-edits branch. Once all of the reviews are done I'll merge this branch into master and make a final release v4.0.0 for the paper.

stuartcampbell commented 5 years ago

Sorry for the delay.

All looks good to me - my review is completed.

One minor suggestion is to consider adding a simple CONTRIBUTING.md, which could just link to the sections in your README.md and CODE_OF_CONDUCT.md - so just to provide a place with a standard name for folks to look. Not essential for this review - just a personal comment.

katyhuff commented 5 years ago

Thanks @stuartcampbell , @myousefi2016 , @mdoucet for your reviews !

@MikeHeiber thank you for a strong submission and for engaging actively in the review process! It looks like there are just a couple of minor suggestions from @stuartcampbell you might consider as you make your 4.0.0 release. Once you have your code and paper all ready in the release, it'll be good to double check the paper, review any lingering details in your code/readme/etc., and then make an archive of the reviewed software in Zenodo/figshare/other service. When you update this thread with the DOI of the archive, I'll move forward with accepting the submission! Until then, now is your moment for final touchups!

MikeHeiber commented 5 years ago

@stuartcampbell Thank you for the suggestion about the contributor instructions file. I had been meaning to add more detailed instructions for how new people could contribute, and your reminder helped give me the activation energy that I needed. There is now both a CONTRIBUTING.md and a CODE_OF_CONDUCT.md file. In addition, I have also added a CHANGELOG.md file as part of my final improvements.

@myousefi2016 and @mdoucet Thank you for taking the time to review my submission.

@katyhuff All of the changes are now merged into the master branch and are contained in the tagged release v4.0.0 that is archived on Zenodo with the following DOI:

10.5281/zenodo.1710685

This DOI needs to be added to the References section of the final paper.

The Changelog lists all changes made between the submitted v4.0.0-rc.2 and the final v4.0.0.

Please let me know if you need any more information from me to move things forward.

katyhuff commented 5 years ago

@whedon generate pdf

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

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

katyhuff commented 5 years ago

@whedon set 10.5281/zenodo.1710685 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.1710685 is the archive.

katyhuff commented 5 years ago

@whedon generate pdf

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

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

katyhuff commented 5 years ago

@whedon generate pdf

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

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

katyhuff commented 5 years ago

@myousefi2016 @mdoucet @stuartcampbell Thank you so much, once again, for your reviews of this work. JOSS relies on reviewers like you!

@MikeHeiber Thanks again for a strong submission, and congratulations, we're ready to accept your paper!

@arfon over to you!

arfon commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/90

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/90, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
arfon commented 5 years ago

@whedon accept deposit=true

whedon commented 5 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 5 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/91
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01072
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? notify your editorial technical team...

arfon commented 5 years ago

@myousefi2016, @stuartcampbell, @mdoucet - many thanks for your reviews and to @katyhuff for editing this submission ✨

@MikeHeiber - your paper is now accepted into JOSS :zap::rocket::boom:

whedon commented 5 years ago

: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 snippets:

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

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

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

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: