search

openjournals / joss-reviews

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

[REVIEW]: riskclustr: Functions to Study Etiologic Heterogeneity #1269

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @zabore (Emily Zabor) Repository: https://github.com/zabore/riskclustr Version: 0.1.0 Editor: @pjotrp Reviewer: @avhoeck Archive: 10.5281/zenodo.2612856

Status

status

Status badge code:

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

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

@avhoeck, 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 @pjotrp know.

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

Review checklist for @avhoeck

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. @avhoeck 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:

pjotrp commented 5 years ago

@avhoeck is our reviewer!

@zabore, we are starting review in this issue tracker. To expedite the review process do you mind going through above list of check boxes and make sure they can be ticked (you can't tick them). Also check the PDF output carefully. Ping us here when you are done.

labarba commented 5 years ago

👋 @avhoeck — Have you been able to get a start on this review? We don't see any tick marks on your checklist. Can you give us a status update?

pjotrp commented 5 years ago

Actually we are waiting for the author to confirm we can tick all boxes. @zabore Emily, do you mind responding?

zabore commented 5 years ago

I am so sorry that I overlooked the initial email requesting me to go over that checklist. I hadn't realized that anything required my attention. I just went through the list, and made one change to the package website to address contributions to the package, everything else was already satisfied. So I believe now that everything in the list can be checked.

Thanks, and sorry again for this oversight. Emily

On Sun, Mar 17, 2019 at 10:00 AM Pjotr Prins notifications@github.com wrote:

Actually we are waiting for the author to confirm we can tick all boxes. @zabore https://github.com/zabore Emily, do you mind responding?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1269#issuecomment-473668549, or mute the thread https://github.com/notifications/unsubscribe-auth/AXKYchvLWnlkNih-Mg2fyDQTwRkbtpCuks5vXkqFgaJpZM4bFW3T .

pjotrp commented 5 years ago

Thank you @zabore. @avhoeck you can start review.

avhoeck commented 5 years ago

Great!

Van: Pjotr Prins [notifications@github.com] Verzonden: maandag 18 maart 2019 16:05 Aan: openjournals/joss-reviews CC: Hoeck, A. van; Mention Onderwerp: Re: [openjournals/joss-reviews] [REVIEW]: riskclustr: Functions to Study Etiologic Heterogeneity (#1269)

Thank you @zaborehttps://github.com/zabore. @avhoeckhttps://github.com/avhoeck you can start review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/1269#issuecomment-473948673, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AY6ZZWJnJ7YEBP3mKw0x-TfIpNRM8a9Lks5vX6sjgaJpZM4bFW3T.

De informatie opgenomen in dit bericht kan vertrouwelijk zijn en is uitsluitend bestemd voor de geadresseerde. Indien u dit bericht onterecht ontvangt, wordt u verzocht de inhoud niet te gebruiken en de afzender direct te informeren door het bericht te retourneren. Het Universitair Medisch Centrum Utrecht is een publiekrechtelijke rechtspersoon in de zin van de W.H.W. (Wet Hoger Onderwijs en Wetenschappelijk Onderzoek) en staat geregistreerd bij de Kamer van Koophandel voor Midden-Nederland onder nr. 30244197.

Denk s.v.p aan het milieu voor u deze e-mail afdrukt.

This message may contain confidential information and is intended exclusively for the addressee. If you receive this message unintentionally, please do not use the contents but notify the sender immediately by return e-mail. University Medical Center Utrecht is a legal person by public law and is registered at the Chamber of Commerce for Midden-Nederland under no. 30244197.

Please consider the environment before printing this e-mail.

pjotrp commented 5 years ago

How are you @avhoeck?

avhoeck commented 5 years ago

This is a nice package that builds on functionalities from previous work developed by the publishing author. Overall, this submission fulfills all requirements as stated by https://joss.readthedocs.io/en/latest/review_criteria.html and I'm ready to sign off as it is.

Some minor changes that could improve the submission:

Previous work focusing on etiologic heterogeneity that has been published by E. Zabor (doi: 10.1002/sim.7405 and 10.1002/sim.5902) was together with C. Begg. If C. Begg has no contributions in this work, the authorship button can be signed off.

On http://www.emilyzabor.com/riskclustr/ website under “reference”, I would expect to see the manuscript DOI or URL, and not an overview of all the riskcluser core functions. Moreover, it would be nice to include (either under “reference” or on the main page) a link to the riskclustr manuscript anyway which is a good resource for the user.

The source code is explicit including bug reporting and extra clarifications for basically every step. I have successfully installed the package and no errors appeared upon testing. However, the simulated data is a bit too basic for me. Would it be possible to rename the column names of simulated data frame (eg X1… into dummy risk factors and Y1-Y300 into dummy sample/patient names?). Moreover, I dont see whether Y1-Y300 scores are actually taken into account in any of the functionalities. If not, I would remove these from the simulated dataframe, an otherwise, I would include an extra argument that refers to observed sample scores for the functions that need sample observations (I guess all?). Ideally, would it be feasible to include Cancer and Steroid Hormone (CASH) study and the Womens’ Contraceptive and Reproductive Experiences (CARE) real you analysed previously to analyse “real-world” datasets.

Thanks for your work! All the best, Arne

zabore commented 5 years ago

Arne, Thank you for these comments. C. Begg did not make any contribution to the development of this software package, though you are correct that he contributed to the original methodology. I appreciate your comment about the website header "Reference" and will change this, and link elsewhere to the methodology paper, likely in the introduction/overview. With respect to the simulated data, the continuous tumor markers, currently labeled y1-y30, are used to demonstrate functionality of the "optimal_kmeans_d" function in "Tutorial: Identify the optimal D subtype solution". If you think it would help, I could rename the x variables to start with "rf" for risk factor and the y variables to start with "tm" for tumor marker if this would increase clarity, but it's true that these simulated data are quite generic regardless and for demonstration purposes only. Unfortunately neither the CASH nor CARE data can be made public at this time.

Thanks again, Emily

On Tue, Mar 26, 2019 at 6:05 AM Arne Van Hoeck notifications@github.com wrote:

This is a nice package that builds on functionalities from previous work developed by the publishing author. Overall, this submission fulfills all requirements as stated by https://joss.readthedocs.io/en/latest/review_criteria.html and I'm ready to sign off as it is.

Some minor changes that could improve the submission:

Previous work focusing on etiologic heterogeneity that has been published by E. Zabor (doi: 10.1002/sim.7405 and 10.1002/sim.5902) was together with C. Begg. If C. Begg has no contributions in this work, the authorship button can be signed off.

On http://www.emilyzabor.com/riskclustr/ website under “reference”, I would expect to see the manuscript DOI or URL, and not an overview of all the riskcluser core functions. Moreover, it would be nice to include (either under “reference” or on the main page) a link to the riskclustr manuscript anyway which is a good resource for the user.

The source code is explicit including bug reporting and extra clarifications for basically every step. I have successfully installed the package and no errors appeared upon testing. However, the simulated data is a bit too basic for me. Would it be possible to rename the column names of simulated data frame (eg X1… into dummy risk factors and Y1-Y300 into dummy sample/patient names?). Moreover, I dont see whether Y1-Y300 scores are actually taken into account in any of the functionalities. If not, I would remove these from the simulated dataframe, an otherwise, I would include an extra argument that refers to observed sample scores for the functions that need sample observations (I guess all?). Ideally, would it be feasible to include Cancer and Steroid Hormone (CASH) study and the Womens’ Contraceptive and Reproductive Experiences (CARE) real you analysed previously to analyse “real-world” datasets.

Thanks for your work! All the best, Arne

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1269#issuecomment-476555093, or mute the thread https://github.com/notifications/unsubscribe-auth/AXKYcoXiRDvNjRoqQFvkKoYGriEcZyu4ks5vafDtgaJpZM4bFW3T .

pjotrp commented 5 years ago

@zabore ping me when you are ready.

avhoeck commented 5 years ago

Thanks for your answer. Signed off the authorship as well.

All the best, Arne

zabore commented 5 years ago

Good morning, I have edited the README to included the main references for the methodology, and I changed the website heading from "Reference" to "Function documentation" for clarity. I left the example data variables as they were, as I am not sure that renaming them adds much. But if I can add a real world dataset example in the future I will certainly do so.

Thank you, Emily

On Wed, Mar 27, 2019 at 4:34 AM Arne Van Hoeck notifications@github.com wrote:

Thanks for your answer. Signed off the authorship as well.

All the best, Arne

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1269#issuecomment-477036873, or mute the thread https://github.com/notifications/unsubscribe-auth/AXKYcntBxCLZNv7wfIZ373OGZIGPZJgpks5vay0LgaJpZM4bFW3T .

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

pjotrp commented 5 years ago

@zabore to finalize your submission and accept your paper in JOSS, we need two things. First, can you confirm that all references in your bibliography have a DOI in the bibliography (if one exists).

Second, we need you to deposit a copy of your software repository (including any revisions made during the JOSS review process) with a data-archiving service.

To do so:

  1. Create a GitHub release of the current version of your software repository
  2. Deposit that release with Zenodo, figshare, or a similar DOI issuer.
  3. Post a comment here with the DOI for the release.
zabore commented 5 years ago

I checked the proofs and noticed one typo, just a missing space between two words, and I pushed a change to correct this in the paper.

I confirm that all references in the bibliography that have an available DOI have it listed.

I have created a GitHub release and deposited it on Zenodo. DOI: 10.5281/zenodo.2612856

Please let me know if you need anything else from me.

Thank you! Emily

On Wed, Mar 27, 2019 at 2:43 PM Pjotr Prins notifications@github.com wrote:

@zabore https://github.com/zabore to finalize your submission and accept your paper in JOSS, we need two things. First, can you confirm that all references in your bibliography have a DOI in the bibliography (if one exists).

Second, we need you to deposit a copy of your software repository (including any revisions made during the JOSS review process) with a data-archiving service.

To do so:

  1. Create a GitHub release https://help.github.com/articles/creating-releases/ of the current version of your software repository
  2. Deposit that release with Zenodo https://zenodo.org/, figshare https://figshare.com/, or a similar DOI issuer.
  3. Post a comment here with the DOI for the release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1269#issuecomment-477299014, or mute the thread https://github.com/notifications/unsubscribe-auth/AXKYcru0F4Z2yJOqTS5rxS6EuZaQyq66ks5va7vUgaJpZM4bFW3T .

pjotrp commented 5 years ago

@whedon set 10.5281/zenodo.2612856 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2612856 is the archive.

pjotrp commented 5 years ago

ping eic @openjournals/joss-eics

danielskatz commented 5 years ago

@whedon accept

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

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - tmp/1269 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/lib/whedon/processor.rb:57:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/bin/whedon:73:in compile' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-01ece1d1d135/bin/whedon:116:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

danielskatz commented 5 years ago

👋 @zabore - something in your change causes the paper to no longer compile.

Please try to fix it - once you have made a change, you can enter @whedon generate pdf as a new comment here. Once you are happy that the paper compiles and is correct, please let me know, and I'll continue the acceptance process.

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

danielskatz commented 5 years ago

👋 @zabore - never mind - the problem is not in the paper.

danielskatz commented 5 years ago

👋 @arfon - can you look at this and see what's failing in whedon's accept functionality?

danielskatz commented 5 years ago

trying again in case it was a one-time problem...

danielskatz 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/585

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

@whedon accept deposit=true
danielskatz commented 5 years ago

👋 @arfon - never mind, the problem seems to have been transient.

danielskatz 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/586
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01269
  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...

danielskatz commented 5 years ago

Thanks to @pjotrp for editing and @avhoeck for reviewing!!

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

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

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

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:

labarba commented 5 years ago

This XML deposit has a lot of false citations, coming from the .bib file, and which are not cited in the paper. I've observed several instances of this lately. I don't know how serious it is.

danielskatz commented 5 years ago

I don't understand this - I thought the PDF generation process would only add to the references when there was a citation in the paper, like bibtex normally does

labarba commented 5 years ago

@arfon — Please clarify this issue: when authors have many entries in their .bib file, they get picked up by whedon and added to the XML, which presumably enters false citations for those works in Crossref.

labarba commented 5 years ago

@danielskatz — I always visually check the XML created after @whedon accept and look at the list of citations, then inspect the PDF and count the citations. Sometimes they don't match, and I go back to the authors and ask them to edit the .bib file.

labarba commented 5 years ago

This XML file has 116 citations entries ... https://github.com/openjournals/joss-papers/blob/bea64c63597ea5a91ad64dc6e622e6eada7c7bcd/joss.01269/10.21105.joss.01269.crossref.xml#L425

danielskatz commented 5 years ago

It seems a bit awkward, as most authors probably have a .bib file with lots of entries that they don't expect to all show up in the metadata. Maybe this indicates a bug in whedon?

arfon commented 5 years ago

It seems a bit awkward, as most authors probably have a .bib file with lots of entries that they don't expect to all show up in the metadata. Maybe this indicates a bug in whedon?

Let's file this as a bug with Whedon. I don't have an immediate fix unfortunately as we currently take whatever is in the bibtex and generate the Crossref citation metadata based on that. For now, we need to do the check that @labarba is suggesting sorry.

danielskatz commented 5 years ago

👋 @zabore - Can you remove the entries from your .bib file that are not actually being cited in the paper? Then we will regenerate the backend XML - This should not change your paper or anything visible to a reader/user.