search

openjournals / joss-reviews

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

[REVIEW]: straditize: Digitizing stratigraphic diagrams #1216

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @Chilipp (Philipp S. Sommer) Repository: https://github.com/Chilipp/straditize Version: v0.1.1 Editor: @yochannah Reviewer: @ixjlyons, @sgrieve Archive: 10.5281/zenodo.2565036

Status

status

Status badge code:

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

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

@ixjlyons & @sgrieve, 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 @yochannah know.

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

Review checklist for @ixjlyons

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @sgrieve

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. @ixjlyons, 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

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

Chilipp commented 5 years ago

👉 Check article proof 📄 👈

seems all right to me. Thanks at @sgrieve and @ixjlyons for being willing to review this package. Looking forward to your feedbacks!

ixjlyons commented 5 years ago

Ok, my review is complete. Nice work @Chilipp!

Overall, straditize appears to be in excellent shape. Aside from a couple installation issues (filed against the target repository), all of the review criteria appear to me to be met. Beyond those, the code looks clean and well-written. I'm not all that familiar with digitization, but adding to the kinds of plots that can be "reverse-engineered" definitely seems worthwhile. The paper conveys that sentiment well and summarizes the package adequately. The documentation looks good standalone, but it shines inside the actual GUI and the integration with the tutorials is excellent.

Once the two installation issues I ran into are addressed, I recommend this submission be accepted.

Chilipp commented 5 years ago

Dear @ixjlyons, thank you very much for your time and your helpful feedback! I already implemented the suggested changes will merge them soon.

sgrieve commented 5 years ago

Hi @Chilipp, @yochannah Below is my review, as noted below I have also opened an issue in the straditize repo regarding the tests.

Firstly I would like to thank the author for this contribution. I have been very impressed by the quality of the software presented here, as well as it's potential utility to a wide range of users across disciplines. I am happy to recommend publication, pending the resolution of the issues outlined below.

Installation

I am performing this review on MacOS Sierra 10.12.6, and found no problems with the conda install as outlined in the documentation. However, one thing I did note is that the quick install instructions on the GitHub readme does not recommend installing in a separate virtual environment. I feel that installing into a virtual environment as the default quickstart install option is probably better than potentially causing problems in someones base environment.

I had the same problem as @ixjlyons when trying to install via pip and from source, that I needed to manually install PyQt. It seems that you have already addressed this, but I just wanted to highlight that it was an issue on my environment as well.

Functionality

Having worked through the tutorials I am happy to confirm that straditize works as described. More generally, I believe that this will be a significant tool for scientists in a range of disciplines, and the way that the software has been designed suggests to me that it will be straightforward to add more diagram types to straditize in future based on community need.

Documentation

I am very impressed by the level and quality of the documentation, in particular the integration of the documentation and tutorials into the GUI. Aside from the broken links mentioned elsewhere in this review, I noted a typo on the first line of this section: https://straditize.readthedocs.io/en/latest/installing.html#running-the-tests

We us should be We use.

Community guidelines

I just wanted to put a note here that your contribution/community guidelines are excellent. I will return to these the next time I have to write some for one of my projects. As above, I noted some broken links, but these have all been addressed in PR 7.

Tests

I have not been able to get the tests to pass on my machine. I have opened an issue to outline this further: https://github.com/Chilipp/straditize/issues/8

It may be useful to add some detail of what a user can expect when running these tests, for example the number of tests that will be run, what does a successful test run look like, are any tests skipped, the approximate duration of the tests.

Paper

The paper is very well written and clearly outlines the scientific problem the author is trying to address, as well as how straditize specifically addresses these issues. The format of the paper follows the general JOSS style, and I believe that all of the relevant literature and materials have been correctly cited.

In conclusion, I have thoroughly enjoyed the experience of reviewing this paper and would like to commend the author for their diligent work to address a pressing concern in our field. I am hoping to introduce this tool to some of my students in the future and look forward to using it in my own research. :rocket:

yochannah commented 5 years ago

Wow, thanks @ixjlyons and @sgrieve for your thorough reviews, and @Chilipp for acting on the feedback so swiftly!

@sgrieve @Chilipp let me know when you two have managed to resolve the testing issue and are ready to go ahead! 👍

Chilipp commented 5 years ago

Dear @yochannah, @sgrieve and @ixjlyons,

thank you very much for your nice words, helpful feedbacks and the reported issues. I apologize for the inconveniences with pip and also with the tests @sgrieve. Unfortunately I have no good methodology to test such a GUI-based application. The unittest framework requires creating and closing lot's of widgets which produces quite some overhead (this especially causes problems on the CI services with very limited RAM, such as OSX on Travis). I would like to address these problems first before merging the suggested changes. But this should be done by Monday.

Chilipp commented 5 years ago

Dear @yochannah, @sgrieve and @ixjlyons,

thanks again for all the input and your fast responses! All reported issues (https://github.com/Chilipp/straditize/issues/1, https://github.com/Chilipp/straditize/issues/2, https://github.com/Chilipp/straditize/issues/3, https://github.com/Chilipp/straditize/issues/4 and https://github.com/Chilipp/straditize/issues/8) are closed and from my side, I am ready to make a new release with a fresh DOI. Is this okay for you?

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

yochannah commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- http://doi.org/10.1109/MCSE.2007.55 is OK
- http://doi.org/10.21105/joss.00363 is OK
- http://doi.org/10.7717/peerj.453 is OK
- http://doi.org/10.1016/j.quascirev.2007.04.007 is OK

MISSING DOIs

- None

INVALID DOIs

- None
yochannah commented 5 years ago

Okay, I've given it a finalish proofread and the text looks great 💯 . One strange thing I noted is that the figure appears in the middle of the reference section though - @arfon any idea what is up?

yochannah commented 5 years ago

@sgrieve and @ixjlyons, y'all each have one checkbox left unticked. If you're happy to accept, can you check those boxes and make a comment once you're done?

@Chilipp Once those last two checkboxes are complete, please do make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive! It's probably worth giving the manuscript a final proofread if you feel you need to, as well. 👍

Thanks all, it's been really positive watching this all come together! 🥇

arfon commented 5 years ago

@arfon any idea what is up?

@yochannah - this is standard LaTeX image layout issues. If the authors make this change the image placement is better: https://github.com/Chilipp/straditize/pull/9

Chilipp commented 5 years ago

Thanks @arfon! The PR has been merged.

@whedon generate pdf

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

ixjlyons commented 5 years ago

@yochannah I checked the last box assuming @Chilipp will make a release on PyPI soon based on the recent fixes so pip install straditize Just Works™ per the installation instructions. Should a new release be made before getting a DOI?

sgrieve commented 5 years ago

@yochannah I've checked the final box, after the issue in the straditize repo was closed I forgot to do it...

Chilipp commented 5 years ago

All right @yochannah, I made a new version 0.1.1 that is available on PyPi and conda and created a release on zenodo:

DOI

It's probably worth giving the manuscript a final proofread if you feel you need to, as well

Also for the manuscript everything looks all right.

yochannah commented 5 years ago

@whedon commands

whedon commented 5 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to accept the paper and deposit with Crossref
@whedon accept

# Ask Whedon to check the references for missing DOIs
@whedon check references
yochannah commented 5 years ago

@Chilipp okay, looking good - one last thing before we accept - can you make sure the Zenodo archive title is the same as the paper title? Thanks!

yochannah commented 5 years ago

@whedon set v0.1.1 as version

whedon commented 5 years ago

OK. v0.1.1 is the version.

Chilipp commented 5 years ago

@Chilipp okay, looking good - one last thing before we accept - can you make sure the Zenodo archive title is the same as the paper title? Thanks!

All right, done

https://zenodo.org/record/2565036#.XGmbPNF7mM8

yochannah commented 5 years ago

@whedon set 10.5281/zenodo.2565036 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2565036 is the archive.

yochannah commented 5 years ago

@openjournals/joss-eics - we're ready to accept this one!! 🎉 🏆

@Chilipp Thanks for your great work on this package and fantastic collaboration with the reviewers.

@sgrieve @ixjlyons Thank you for your fantastic and in-depth reviews.

Chilipp commented 5 years ago

Thank you very much @yochannah and especially @sgrieve and @ixjlyons for your helpful inputs! I am very glad that everything worked out so well 😃

arfon commented 5 years ago

@whedon accept

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

OK DOIs

- http://doi.org/10.1109/MCSE.2007.55 is OK
- http://doi.org/10.21105/joss.00363 is OK
- http://doi.org/10.7717/peerj.453 is OK
- http://doi.org/10.1016/j.quascirev.2007.04.007 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 5 years ago

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

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/501, 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/502
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01216
  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

@ixjlyons, @sgrieve - many thanks for your reviews and to @yochannah for editing this submission ✨

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

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

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

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: