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]: ungroup: An R package for efficient estimation of smooth distributions from coarsely binned data #937

Closed
whedon opened this Issue Sep 11, 2018 · 33 comments

Comments

Projects
None yet
5 participants
@whedon
Collaborator

whedon commented Sep 11, 2018

Submitting author: @mpascariu (Marius Dan Pascariu)
Repository: https://github.com/mpascariu/ungroup
Version: v1.0.3
Editor: @karthik
Reviewer: @rlbarter
Archive: 10.5281/zenodo.1421648

Status

status

Status badge code:

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

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

@rlbarter, 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 @karthik know.

Please try and complete your review in the next two weeks

Review checklist for @rlbarter

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.3)?
  • Authorship: Has the submitting author (@mpascariu) 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 11, 2018

Collaborator

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

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

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

whedon commented Sep 11, 2018

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

This comment has been minimized.

Show comment
Hide comment
@whedon
Collaborator

whedon commented Sep 11, 2018

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 12, 2018

Thank you @rlbarter for agreeing to review the ungroup package. I am looking forward to your comments and suggestions.

mpascariu commented Sep 12, 2018

Thank you @rlbarter for agreeing to review the ungroup package. I am looking forward to your comments and suggestions.

@rlbarter

This comment has been minimized.

Show comment
Hide comment
@rlbarter

rlbarter Sep 17, 2018

Collaborator

Just a few minor revision comments:

  • The CRAN version is 1.0.0 and the github version is 1.0.4 (according to the DESCRIPTION file). This review is theoretically for version 1.0.3. Not sure if this matters much (@karthik can you weigh in on this?)
  • I couldn't find examples of how to use the package on the GitHub page. Perhaps you could provide a link to the vignette and reference manual (available on CRAN) in the GitHub README.md file
  • There are currently no guidelines for contributing other than a statement on the GitHub README that says: "You can track (and contribute to) the development of ungroup at https://github.com/mpascariu/ungroup".
    I recommend proving more detailed information on guidelines for contributing (e.g. how to Contribute to the software, Report issues or problems with the software, Seek support)
  • I'm currently taking it on faith that the software does what it claims to. From the description, I don't really understand what it means to ungroup a histogram, so I don't know how to check that the functions are indeed doing this. The example from the CRAN vignette didn't seem to visually change the histogram output (but I was unsure if it was supposed to - it seems a strange example to use if it isn't supposed to change though):
M1 <- pclm(x, y, nlast)
plot(M1)
M2 <- pclm(x, y, nlast, out.step = 0.5)
plot(M2)
Collaborator

rlbarter commented Sep 17, 2018

Just a few minor revision comments:

  • The CRAN version is 1.0.0 and the github version is 1.0.4 (according to the DESCRIPTION file). This review is theoretically for version 1.0.3. Not sure if this matters much (@karthik can you weigh in on this?)
  • I couldn't find examples of how to use the package on the GitHub page. Perhaps you could provide a link to the vignette and reference manual (available on CRAN) in the GitHub README.md file
  • There are currently no guidelines for contributing other than a statement on the GitHub README that says: "You can track (and contribute to) the development of ungroup at https://github.com/mpascariu/ungroup".
    I recommend proving more detailed information on guidelines for contributing (e.g. how to Contribute to the software, Report issues or problems with the software, Seek support)
  • I'm currently taking it on faith that the software does what it claims to. From the description, I don't really understand what it means to ungroup a histogram, so I don't know how to check that the functions are indeed doing this. The example from the CRAN vignette didn't seem to visually change the histogram output (but I was unsure if it was supposed to - it seems a strange example to use if it isn't supposed to change though):
M1 <- pclm(x, y, nlast)
plot(M1)
M2 <- pclm(x, y, nlast, out.step = 0.5)
plot(M2)
@karthik

This comment has been minimized.

Show comment
Hide comment
@karthik

karthik Sep 18, 2018

Collaborator

@rlbarter Good to check the latest version. The bump from 1.0.3 to 1.0.4 appears to be in response to my suggestions (most were cosmetic) which should not change any of your review comments. You can view those changes here: mpascariu/ungroup@964b126

Collaborator

karthik commented Sep 18, 2018

@rlbarter Good to check the latest version. The bump from 1.0.3 to 1.0.4 appears to be in response to my suggestions (most were cosmetic) which should not change any of your review comments. You can view those changes here: mpascariu/ungroup@964b126

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 18, 2018

@rlbarter Thanks for your comments. See below my response.

  1. In the README.md it is specified the version no. of the stable software available on CRAN. The development version will change more frequently and has no real importance to the common user.

  2. In GitHub we provide all the source file. The .rmd file can be found in ungroup/vignettes/ folder. The already compiled vignette can be found in ungroup/inst/doc/Intro.pdf. We will add a link in README.md if this solves the situation.

  3. The guidelines for contributing can be found in CONTRIBUTING.md file.

The last point I will address in the next comment.

mpascariu commented Sep 18, 2018

@rlbarter Thanks for your comments. See below my response.

  1. In the README.md it is specified the version no. of the stable software available on CRAN. The development version will change more frequently and has no real importance to the common user.

  2. In GitHub we provide all the source file. The .rmd file can be found in ungroup/vignettes/ folder. The already compiled vignette can be found in ungroup/inst/doc/Intro.pdf. We will add a link in README.md if this solves the situation.

  3. The guidelines for contributing can be found in CONTRIBUTING.md file.

The last point I will address in the next comment.

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 18, 2018

  1. ... I don't really understand what it means to ungroup a histogram
    To ungroup a histogram means to disaggregate the data behind that histogram in order to build a new one on a finer grid.

Consider the following problem:
A study group consists of 20 students. 15 of them are between 15 and 17 years old. 5 students are 18 or older. What is the average age of the students?

Based on the info above we can not calculate the average age unless we know the age of every student. However, we can approximate it by building a histogram with 2 bars (15-17 and 18+) and say that the average age is a weighted mean of the midpoints of the two groups (16 and 19). This implies that the student ages are uniformly distributed in the intervals and the open interval closes at age 20 (this is a naive approximation since we can also have a 35yo student and no 16yo's).

Now, if we would be able somehow to build a histogram with more bars, say 6 bars (15, 16, 17, 18, 19 and 20+) that better describes the reality, we would be able to come up with a more accurate approximation of the average age. This is what ungroup does.

For more real-world example and explanation of the method see this article: https://academic.oup.com/aje/article/182/2/138/94562

Regarding the two examples, the difference is seen in the estimated groups and estimated counts:

> # Example 1 ----------------------
> M1 <- pclm(x, y, nlast)
> head(fitted(M1))
     [0,1)      [1,2)      [2,3)      [3,4)      [4,5)      [5,6) 
292.254945  47.567040  12.031104   5.101512   3.653694   3.801854 
> # Example 2 ----------------------
> # ungroup even in smaller intervals
> M2 <- pclm(x, y, nlast, out.step = 0.5)
> head(fitted(M2))
   [0,0.5)    [0.5,1)    [1,1.5)    [1.5,2)    [2,2.5)    [2.5,3) 
211.751314  80.583505  32.679931  14.663353   7.463173   4.379769 

Note, in example 1 we are estimating intervals of length 1. In example 2 we are estimating intervals of length 0.5 using the same aggregate data.

Visually the difference can be seen like this:

> plot(M1, type = "s")
> plot(M2, type = "s")

mpascariu commented Sep 18, 2018

  1. ... I don't really understand what it means to ungroup a histogram
    To ungroup a histogram means to disaggregate the data behind that histogram in order to build a new one on a finer grid.

Consider the following problem:
A study group consists of 20 students. 15 of them are between 15 and 17 years old. 5 students are 18 or older. What is the average age of the students?

Based on the info above we can not calculate the average age unless we know the age of every student. However, we can approximate it by building a histogram with 2 bars (15-17 and 18+) and say that the average age is a weighted mean of the midpoints of the two groups (16 and 19). This implies that the student ages are uniformly distributed in the intervals and the open interval closes at age 20 (this is a naive approximation since we can also have a 35yo student and no 16yo's).

Now, if we would be able somehow to build a histogram with more bars, say 6 bars (15, 16, 17, 18, 19 and 20+) that better describes the reality, we would be able to come up with a more accurate approximation of the average age. This is what ungroup does.

For more real-world example and explanation of the method see this article: https://academic.oup.com/aje/article/182/2/138/94562

Regarding the two examples, the difference is seen in the estimated groups and estimated counts:

> # Example 1 ----------------------
> M1 <- pclm(x, y, nlast)
> head(fitted(M1))
     [0,1)      [1,2)      [2,3)      [3,4)      [4,5)      [5,6) 
292.254945  47.567040  12.031104   5.101512   3.653694   3.801854 
> # Example 2 ----------------------
> # ungroup even in smaller intervals
> M2 <- pclm(x, y, nlast, out.step = 0.5)
> head(fitted(M2))
   [0,0.5)    [0.5,1)    [1,1.5)    [1.5,2)    [2,2.5)    [2.5,3) 
211.751314  80.583505  32.679931  14.663353   7.463173   4.379769 

Note, in example 1 we are estimating intervals of length 1. In example 2 we are estimating intervals of length 0.5 using the same aggregate data.

Visually the difference can be seen like this:

> plot(M1, type = "s")
> plot(M2, type = "s")
@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 18, 2018

@rlbarter Your suggestions have been addressed in v1.0.5 now on GitHub. Thanks.

mpascariu commented Sep 18, 2018

@rlbarter Your suggestions have been addressed in v1.0.5 now on GitHub. Thanks.

@rlbarter

This comment has been minimized.

Show comment
Hide comment
@rlbarter

rlbarter Sep 18, 2018

Collaborator

@mpascariu Great. This all looks good to me! Up to you whether you want to add a URL link to the vignette in the README (I personally find this helpful in general when I go to R packages on GitHub).

Collaborator

rlbarter commented Sep 18, 2018

@mpascariu Great. This all looks good to me! Up to you whether you want to add a URL link to the vignette in the README (I personally find this helpful in general when I go to R packages on GitHub).

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 18, 2018

@rlbarter Excellent. Thank you very much.
I added the link too.

mpascariu commented Sep 18, 2018

@rlbarter Excellent. Thank you very much.
I added the link too.

@karthik

This comment has been minimized.

Show comment
Hide comment
@karthik

karthik Sep 18, 2018

Collaborator

@whedon generate pdf

Collaborator

karthik 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

@karthik

This comment has been minimized.

Show comment
Hide comment
@karthik

karthik Sep 18, 2018

Collaborator

@mpascariu Before I accept, can you please archive this version of the software on Zenodo and post a a DOI for me please?

Collaborator

karthik commented Sep 18, 2018

@mpascariu Before I accept, can you please archive this version of the software on Zenodo and post a a DOI for me please?

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 19, 2018

@karthik The Zenodo doi is: 10.5281/zenodo.1421648

As discussed with @arfon in PRE REVIEW, before publishing the article is it possible to place the Figure 1 before the 3rd paragraph and the Acknowledgements at the end, after Figure 2? I am not sure how to fix this in a markdown file. As it looks now the Acknowledgements are dividend between page 1 and 3.

Thanks.

mpascariu commented Sep 19, 2018

@karthik The Zenodo doi is: 10.5281/zenodo.1421648

As discussed with @arfon in PRE REVIEW, before publishing the article is it possible to place the Figure 1 before the 3rd paragraph and the Acknowledgements at the end, after Figure 2? I am not sure how to fix this in a markdown file. As it looks now the Acknowledgements are dividend between page 1 and 3.

Thanks.

@karthik

This comment has been minimized.

Show comment
Hide comment
@karthik

karthik Sep 19, 2018

Collaborator

@mpascariu As @arfon noted earlier, it is very challenging to typeset accurately when using markdown + pandoc. Given that you also have such a short paper the accurate placement you seek would be very challenging (without inserting a lot of empty space and page breaks). You might have to mess with figure sizes to make it fit the way you seek.

I'll let @arfon weigh in but I think the paper looks fine the way it is.

Collaborator

karthik commented Sep 19, 2018

@mpascariu As @arfon noted earlier, it is very challenging to typeset accurately when using markdown + pandoc. Given that you also have such a short paper the accurate placement you seek would be very challenging (without inserting a lot of empty space and page breaks). You might have to mess with figure sizes to make it fit the way you seek.

I'll let @arfon weigh in but I think the paper looks fine the way it is.

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 19, 2018

@karthik I understand.

I made some minor changes in the .md file. I think the paper should look better now.
Can we generate the PDF again? Thank you.

mpascariu commented Sep 19, 2018

@karthik I understand.

I made some minor changes in the .md file. I think the paper should look better now.
Can we generate the PDF again? Thank you.

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 19, 2018

Member

@whedon generate pdf

Member

arfon commented Sep 19, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 19, 2018

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

whedon commented Sep 19, 2018

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

This comment has been minimized.

Show comment
Hide comment
@whedon
Collaborator

whedon commented Sep 19, 2018

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 19, 2018

Member

@mpascariu - you can ask @whedon to recompile the paper for you with @whedon generate pdf

Also, it looks like the #Summary needs changing to # Summary at the top of the paper.

Member

arfon commented Sep 19, 2018

@mpascariu - you can ask @whedon to recompile the paper for you with @whedon generate pdf

Also, it looks like the #Summary needs changing to # Summary at the top of the paper.

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 19, 2018

@arfon It is corrected now.

mpascariu commented Sep 19, 2018

@arfon It is corrected now.

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu commented Sep 20, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 20, 2018

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

whedon commented Sep 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@whedon
Collaborator

whedon commented Sep 20, 2018

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 20, 2018

@arfon I think the article looks fine now. Thank you very much.

mpascariu commented Sep 20, 2018

@arfon I think the article looks fine now. Thank you very much.

@karthik

This comment has been minimized.

Show comment
Hide comment
@karthik

karthik Sep 20, 2018

Collaborator

@whedon set 10.5281/zenodo.1421648 as archive

Collaborator

karthik commented Sep 20, 2018

@whedon set 10.5281/zenodo.1421648 as archive

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 20, 2018

Collaborator

OK. 10.5281/zenodo.1421648 is the archive.

Collaborator

whedon commented Sep 20, 2018

OK. 10.5281/zenodo.1421648 is the archive.

@karthik

This comment has been minimized.

Show comment
Hide comment
@karthik

karthik Sep 20, 2018

Collaborator

@mpascariu Thank you for the submission! And thanks @rlbarter for the review! This is ready to accept @arfon 🎉

Collaborator

karthik commented Sep 20, 2018

@mpascariu Thank you for the submission! And thanks @rlbarter for the review! This is ready to accept @arfon 🎉

@arfon arfon added the accepted label Sep 20, 2018

@mpascariu

This comment has been minimized.

Show comment
Hide comment
@mpascariu

mpascariu Sep 20, 2018

Thank you everyone for your work! It is much appreciated.

mpascariu commented Sep 20, 2018

Thank you everyone for your work! It is much appreciated.

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 20, 2018

Member

@rlbarter - many thanks for your review here and to @karthik for editing this submission

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

Member

arfon commented Sep 20, 2018

@rlbarter - many thanks for your review here and to @karthik for editing this submission

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

@arfon arfon closed this Sep 20, 2018

@whedon

This comment has been minimized.

Show comment
Hide comment
@whedon

whedon Sep 20, 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.00937/status.svg)](https://doi.org/10.21105/joss.00937)

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

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

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

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

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

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