Closed whedon closed 3 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @lisamr, @lwjohnst86 it looks like you're currently assigned to review this paper :tada:.
:warning: JOSS reduced service mode :warning:
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
:star: Important :star:
If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/jose-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:
For a list of things I can do to help you, just type:
@whedon commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@whedon generate pdf
Failed to discover a Statement of need
section in paper
Software report (experimental):
github.com/AlDanial/cloc v 1.88 T=0.83 s (119.8 files/s, 22799.4 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
HTML 36 1775 445 7021
SVG 20 0 1 3457
Markdown 7 439 0 1306
R 21 291 1059 1129
CSS 3 99 48 428
JavaScript 4 65 35 267
Rmd 2 214 322 176
YAML 4 19 1 155
TeX 1 9 1 71
Bourne Shell 1 1 4 1
-------------------------------------------------------------------------------
SUM: 99 2912 1916 14011
-------------------------------------------------------------------------------
Statistical information for the repository '3d5a73c19e8a4353347590c3' was
gathered on 2021/05/06.
The following historical commit information, by author, was found:
Author Commits Insertions Deletions % of changes
ismayc 3 187 2 29.58
rudeboybert 7 316 134 70.42
Below are the number of rows from each author that have survived and are still
intact in the current revision:
Author Rows Stability Age % in comments
Chester Ismay 70 100.0 33.1 7.14
rudeboybert 297 94.0 15.6 10.10
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.21105/joss.01686 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@whedon set v0.5.1 as version
OK. v0.5.1 is the version.
@lisamr & @lwjohnst86, the review can now officially start here. Please read from the top of the thread and be sure to review the reviewer guidelines. You'll work your way through the checklists above over the next 4 weeks. Post comments or issues to the main repository as you go along or all at once, your preference. An automatic notice will be sent out in 2 weeks as a reminder. You should review the paper, software documentation, and test out & use the software. The paper authors can (and are encouraged to) interact while you do the review. Let me know if there are any questions. Welcome to JOSE!
Also note that this is should be reviewed as a "educational software tools" submission.
@moorepants I tried to follow this link: "Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations", but I get this message: "Sorry, we couldn't find that repository invitation. It is possible that the invitation was revoked or that you are not logged into the invited account." I'm assuming that's where I can edit the checklist, correct?
I started going through the checklist pasted at the top of this thread:
Among the many useful features of the moderndive package outlined in our essay “Why should you use the moderndive package for intro linear regression?” we highlight three functions in particular as covered there. We also mention the geom_parallel_slopes() function as #4.
Just say there are 4 functions covered. I'm not sure why geom_parallel_slopes
is singled out.
get_regression_table()
I suggest at least in the help documentation, it should be stated what the alpha level is. I know it's probably 0.05, but it should be stated, or even allowed to vary since the point of the function is to understand confidence intervals between a given range.get_regression_points()
: If this package is aimed at students very new to statistics, it might be helpful to include the following in the readme.md file: "...including fitted/predicted values (e.g. 'score_hat')"get_regression_summaries()
: excuse me if I'm wrong, but should df = 2
? The output from summary(score_model)
shows 1 and 461 DF. @moorepants I tried to follow this link: "Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations", but I get this message: "Sorry, we couldn't find that repository invitation. It is possible that the invitation was revoked or that you are not logged into the invited account." I'm assuming that's where I can edit the checklist, correct?
@labarba, do the reviewers have to follow this link? It seems like a joss thing that hasnt' converted to jose.
Functional documentation and tests: I'm not qualified to assess this since I don't write packages (I'm more of a user than developer)
@lisamr, is it possible for your to try running the tests? It's good if each reviewer can do so. You don't have to inspect the code for them.
Also, each reviewer should be able to have a look at this too:
Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
This is checking to see if there are understandable vignettes for each public function, class, etc.
Actually, vignettes may have not been precise enough. There is object documentation, see:
https://r-pkgs.org/man.html#man
and vignettes:
https://r-pkgs.org/vignettes.html#vignettes
Ideally, the package will have both of these types of documentation. The first is the "API method documentation" mentioned in the checklist. The second is a bit more long form explanations that aren't tied to a specific object. The object documentation should be present for each public function and the vignettes should be present as per the reviewers' suggestions, mostly to conform with best practices in R documentation.
Hi @lisamr thank you for bringing up those checklist items. I'll make the necessary changes and have Whedon generate a new pdf today.
@moorepants got it, so the "functionality documentation" checklist item is referring to the object documentation. @rudeboybert below is my feedback:
geom_categorical_model()
: It is not obvious how the solid and dashed lines differ or how they were determined and it's not clearly stated in the documentation. I thought maybe pairwise contrasts were being performed, but when I look at two categories that are not significantly different (e.g. ggplot(evals, aes(pic_outfit, score )) + geom_point() + geom_categorical_model()
), their lines are still different. Then I thought maybe each category gets its own linetype, but when I look at ggplot(mpg, aes(x = drv, y = hwy)) + geom_point() + geom_categorical_model()
, again there are only 2 types. get_regression_summaries()
: All of the columns are self-explanatory except for df. More details under the "Value" section would be nice.get_regression_table()
: Again, either state that the upper and lower bounds represent the 95% confidence interval, or even better, allow the interval ranges to vary, which in my opinion will help break down this (artificial) idea that 95% CI and the associated p-values are all that matter. I went through the paper last night and my first impression was that it was long (~2500 words of text) and overlapped quite a lot with the text and code in the readme file. From what I understand, "Detailed documentation should be present in the repository of the submitted software or module, is reviewed there, and does not need to be included in the paper" (link here). It also lacked a Statement of Need section. I thought a lot of the information presented could have been summed up in a shorter summary paragraph. I also thought the code/output could have been condensed by directing the reader to the repository's documentation.
Here are some key descriptions of the paper, quoted from https://openjournals.readthedocs.io/en/jose/submitting.html:
@moorepants lmk if I'm misunderstanding anything or maybe clarify how the software paper should differ from the readme and other vignettes?
@moorepants lmk if I'm misunderstanding anything or maybe clarify how the software paper should differ from the readme and other vignettes?
Yes, you've interpreted correctly. The documentation and paper should be different as you've described.
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi, I had almost finished addressing the initial comments by @lisamr when they followed up with more feedback. I will address the initial comments only here:
I started going through the checklist pasted at the top of this thread:
General checks
- authorship: It is currently unclear how Max Kuhn contributed, as they are not listed as a contributor on the Github repository and there isn't an author contribution section in the manuscript to clarify.
I added a "Author contributions" section towards the end of the paper.
Documentation
- Statement of need: It's missing in the Readme file and in the MS
A "Statement of need" section has been added at the outset of the manuscript.
- Installation instructions: I couldn't find a clearly stated list of dependencies in the repository, but maybe I was looking in the wrong place. I checked under the "dependency graph" tab by clicking on the "insight" button, and no dependencies were listed there either. https://github.com/moderndive/moderndive/network/dependencies
All the dependencies are automatically installed when installing the moderndive
package, the list of all dependencies can be seen in the DESCRIPTION
file
- Functional documentation and tests: I'm not qualified to assess this since I don't write packages (I'm more of a user than developer). @lwjohnst86 do you want to make sure to cover that component?
@moorepants addressed this above
Minor details on the readme file
- Under Demo, it's stated
Among the many useful features of the moderndive package outlined in our essay “Why should you use the moderndive package for intro linear regression?” we highlight three functions in particular as covered there. We also mention the geom_parallel_slopes() function as #4.
Just say there are 4 functions covered. I'm not sure why
geom_parallel_slopes
is singled out.
That was a typo: The README has been updated to indicate that there are 4 functions highlighted.
- for the function
get_regression_table()
I suggest at least in the help documentation, it should be stated what the alpha level is. I know it's probably 0.05, but it should be stated, or even allowed to vary since the point of the function is to understand confidence intervals between a given range.
Thank you for this great suggestion. We updated the functionality of this function to allow for a conf.level
argument in this commit, and updated the README and manuscript accordingly. This is only available in the development version of the package v0.5.1.9000
for now, I'll wait for more feedback before updating on CRAN. @moorepants could you update the version to v0.5.1.9000
?
- for the function
get_regression_points()
: If this package is aimed at students very new to statistics, it might be helpful to include the following in the readme.md file: "...including fitted/predicted values (e.g. 'score_hat')"
Thank you, similar language was added to the README in this commit
- for the function
get_regression_summaries()
: excuse me if I'm wrong, but shoulddf = 2
? The output fromsummary(score_model)
shows 1 and 461 DF.
You are correct. This seems to have been a bug in the broom::glance()
function that has since been fixed; the README has been updated.
Here are my initial responses to the second round of feedback from @lisamr
@moorepants got it, so the "functionality documentation" checklist item is referring to the object documentation. @rudeboybert below is my feedback:
- documentation for various datasets look great--good detail on explaining each column and providing source of the data
Thank you
geom_categorical_model()
: It is not obvious how the solid and dashed lines differ or how they were determined and it's not clearly stated in the documentation. I thought maybe pairwise contrasts were being performed, but when I look at two categories that are not significantly different (e.g.ggplot(evals, aes(pic_outfit, score )) + geom_point() + geom_categorical_model()
), their lines are still different. Then I thought maybe each category gets its own linetype, but when I look atggplot(mpg, aes(x = drv, y = hwy)) + geom_point() + geom_categorical_model()
, again there are only 2 types.
Since this function is not used in the README nor is it used in the manuscript, would you be ok if I left this function as is? Please let me know if you disagree
get_regression_summaries()
: All of the columns are self-explanatory except for df. More details under the "Value" section would be nice.
The following note is now included in the README in the "Produce metrics on the quality of regression model fits": and the degrees of freedom df
get_regression_table()
: Again, either state that the upper and lower bounds represent the 95% confidence interval, or even better, allow the interval ranges to vary, which in my opinion will help break down this (artificial) idea that 95% CI and the associated p-values are all that matter.
Thanks for this great point. See my comment on the updated get_regression_points()
function in my previous messages.
Larger structural comments on the paper
I went through the paper last night and my first impression was that it was long (~2500 words of text) and overlapped quite a lot with the text and code in the readme file. From what I understand, "Detailed documentation should be present in the repository of the submitted software or module, is reviewed there, and does not need to be included in the paper" (link here). It also lacked a Statement of Need section. I thought a lot of the information presented could have been summed up in a shorter summary paragraph. I also thought the code/output could have been condensed by directing the reader to the repository's documentation.
Thank for this feedback. I have addressed the Statement of Need section in my previous message. Furthermore, the main paper has been cut from ~2500 words to 1026 words with a lot of the content being moved to the repo README
Here are some key descriptions of the paper, quoted from https://openjournals.readthedocs.io/en/jose/submitting.html:
- Write a short Markdown file titled paper.md with title, author names and affiliations, containing a description of the software, statement of need, and key references.
- JOSE papers contain a limited set of metadata, plus Summary & Reference sections. We explicitly do not publish long-form articles, because the scholarship represented by a JOSE article is contained in the software or learning modules themselves. Expected length is around 1000 words max.
@moorepants lmk if I'm misunderstanding anything or maybe clarify how the software paper should differ from the readme and other vignettes?
:wave: @lwjohnst86, please update us on how your review is going (this is an automated reminder).
:wave: @lisamr, please update us on how your review is going (this is an automated reminder).
👋 @lisamr, please update us on how your review is going (this is an automated reminder).
I've gone through the checklist as much as I can and I'm actually still waiting on the invite to this URL: https://github.com/openjournals/joss-reviews/invitations. The next major steps are to review the newly shortened version of the paper.
I'm actually still waiting on the invite to this URL: https://github.com/openjournals/joss-reviews/invitations.
@lisamr I don't think this is important. Don't worry about it. The link is broken as it is for joss, not jose.
I've gone through the checklist as much as I can
You can check the boxes at the top for the things you've approved as complete.
I've gone through the checklist as much as I can
You can check the boxes at the top for the things you've approved as complete.
Hmm, I'm afraid I don't know how to edit the checklist up top. Help?
@whedon re-invite @lisamr as reviewer
The reviewer already has a pending invite.
@lisamr please accept the invite by clicking this link: https://github.com/openjournals/jose-reviews/invitations
There's the correct link! Thanks @labarba.
Hmm, I'm afraid I don't know how to edit the checklist up top. Help?
Presumably when you accept the new link invite you'll be able to check the boxes.
same issue with the new link: "Sorry, we couldn't find that repository invitation. It is possible that the invitation was revoked or that you are not logged into the invited account."
@moorepants maybe we could just zoom and I can share my screen or something. Maybe I'm missing something obvious.
@openjournals/dev Help! We have a reviewer unable to edit the checkboxes. Can you check the issue?
@whedon re-invite @lisamr as reviewer
I'm sorry @arfon, I'm afraid I can't do that. That's something only editors are allowed to do.
OK, could you try this link again @lisamr: https://github.com/openjournals/jose-reviews/invitations
@arfon perfect, that link worked and I can finally check the checkboxes above. Thank you!
@rudeboybert @labarba @lisamr I finished my review. All my issues and feedback were either created as Issues or PRs in the moderndive package repo. Overall looks great! If the issues and PRs are addressed, I would say it's good to go!
Wow @lwjohnst86 thanks for the very thorough and thoughtful reviews, and double thanks for communicating them as issues & PRs directly in the moderndive package repo!
I hope to address @lisamr's outstanding comments and Luke's PR's issues next week. I'll tag you both as well as @moorepants when I'm done. Many thanks!
@lisamr and @lwjohnst86 Thanks for your timely and thorough reviews!
@rudeboybert sounds like you are on top of things, looking forward to your responses and adjustments from the feedback.
As was noted by both reviewers, please adjust the paper to fit the JOSE guidelines more closely. It should not be a repeat of the documentation. All other comments look good to me.
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hello @moorepants @lisamr @lwjohnst86, many thanks for all your work in this review. I believe I've addressed all the outstanding issues, in particular cutting down the manuscript length from ~2500 to 1026 words. Please let me know if I need to do anything else.
@rudeboybert, great to hear you've updated things based on the review. It would be helpful if you list here the things you have changed so we can all see how the various review comments have been addressed. This list should include explanations for the unchecked boxes in the checklist, as well as any other relevant comments.
Hi @moorepants, sounds good. I've broken down my response in 4 sections:
General checks
- authorship: It is currently unclear how Max Kuhn contributed, as they are not listed as a contributor on the Github repository and there isn't an author contribution section in the manuscript to clarify.
I added a "Author contributions" section towards the end of the paper.
Documentation
- Statement of need: It's missing in the Readme file and in the MS
A "Statement of need" section has been added at the outset of the manuscript.
- Installation instructions: I couldn't find a clearly stated list of dependencies in the repository, but maybe I was looking in the wrong place. I checked under the "dependency graph" tab by clicking on the "insight" button, and no dependencies were listed there either. https://github.com/moderndive/moderndive/network/dependencies
All the dependencies are automatically installed when installing the moderndive
package, the list of all dependencies can be seen in the DESCRIPTION
file
- Functional documentation and tests: I'm not qualified to assess this since I don't write packages (I'm more of a user than developer). @lwjohnst86 do you want to make sure to cover that component?
@lwjohnst86 checked this checkbox.
Minor details on the readme file
- Under Demo, it's stated
Among the many useful features of the moderndive package outlined in our essay “Why should you use the moderndive package for intro linear regression?” we highlight three functions in particular as covered there. We also mention the geom_parallel_slopes() function as #4.
Just say there are 4 functions covered. I'm not sure why
geom_parallel_slopes
is singled out.
That was a typo: The README has been updated (in the Basic Usage section) to highlight 4 functions, not 3.
- for the function
get_regression_table()
I suggest at least in the help documentation, it should be stated what the alpha level is. I know it's probably 0.05, but it should be stated, or even allowed to vary since the point of the function is to understand confidence intervals between a given range.
Thank you for this great suggestion. We updated the functionality of this function to allow for a conf.level
argument in this commit, and updated the README and manuscript accordingly. This is only currently available in the development version of the package v0.5.1.9000
for now. This will be included in the next v0.5.2.
version on CRAN.
- for the function
get_regression_points()
: If this package is aimed at students very new to statistics, it might be helpful to include the following in the readme.md file: "...including fitted/predicted values (e.g. 'score_hat')"
Thank you, similar language was added to the README in this commit
- for the function
get_regression_summaries()
: excuse me if I'm wrong, but shoulddf = 2
? The output fromsummary(score_model)
shows 1 and 461 DF.
You are correct. This seems to have been a bug in the broom::glance()
function that has since been fixed; the README has been updated.
@moorepants got it, so the "functionality documentation" checklist item is referring to the object documentation. @rudeboybert below is my feedback:
- documentation for various datasets look great--good detail on explaining each column and providing source of the data
Thank you
geom_categorical_model()
: It is not obvious how the solid and dashed lines differ or how they were determined and it's not clearly stated in the documentation. I thought maybe pairwise contrasts were being performed, but when I look at two categories that are not significantly different (e.g.ggplot(evals, aes(pic_outfit, score )) + geom_point() + geom_categorical_model()
), their lines are still different. Then I thought maybe each category gets its own linetype, but when I look atggplot(mpg, aes(x = drv, y = hwy)) + geom_point() + geom_categorical_model()
, again there are only 2 types.
Since this function is not used in the README nor is it used in the manuscript, I think it's beyond the scope of this review and thus this point can be left unaddressed. Please let me know if you disagree.
get_regression_summaries()
: All of the columns are self-explanatory except for df. More details under the "Value" section would be nice.
The following note is now included in the README in the "Produce metrics on the quality of regression model fits": and the degrees of freedom df
get_regression_table()
: Again, either state that the upper and lower bounds represent the 95% confidence interval, or even better, allow the interval ranges to vary, which in my opinion will help break down this (artificial) idea that 95% CI and the associated p-values are all that matter.
Thanks for this great point. This was addressed above.
Larger structural comments on the paper
I went through the paper last night and my first impression was that it was long (~2500 words of text) and overlapped quite a lot with the text and code in the readme file. From what I understand, "Detailed documentation should be present in the repository of the submitted software or module, is reviewed there, and does not need to be included in the paper" (link here). It also lacked a Statement of Need section. I thought a lot of the information presented could have been summed up in a shorter summary paragraph. I also thought the code/output could have been condensed by directing the reader to the repository's documentation.
Here are some key descriptions of the paper, quoted from https://openjournals.readthedocs.io/en/jose/submitting.html:
- Write a short Markdown file titled paper.md with title, author names and affiliations, containing a description of the software, statement of need, and key references.
- JOSE papers contain a limited set of metadata, plus Summary & Reference sections. We explicitly do not publish long-form articles, because the scholarship represented by a JOSE article is contained in the software or learning modules themselves. Expected length is around 1000 words max.
@moorepants lmk if I'm misunderstanding anything or maybe clarify how the software paper should differ from the readme and other vignettes?
Thanks for this feedback. I have addressed the Statement of Need section in my previous message. Furthermore, the main paper has been cut from ~2500 words to 1026 words with a lot of the content being moved to the repo README
@rudeboybert @labarba @lisamr I finished my review. All my issues and feedback were either created as Issues or PRs in the moderndive package repo. Overall looks great! If the issues and PRs are addressed, I would say it's good to go!
Thanks for filing all your comments and suggestions as GitHub issues and PR's. I will summarize what I did to address all the above referenced issues from 6/4:
A statement of need: Do the authors clearly state the need for this software and who the target audience is?
Added to manuscript
Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
I've addressed all suggestions to clarify points of confusion in help files (where appropriate) or in the README (for more verbose explanations).
A statement of need: Do the authors clearly state the need for this software and who the target audience is?
Added to manuscript
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
Added to main repo.
References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
All books and articles have DOI links. No DOI link existed for the broom package
Thank you for the thorough and clear write up and addressing each point brought up by the reviewers.
@lisamr and @lwjohnst86, can you both take a second pass here at the changes made and report on whether this addresses your prior comments? Please check off the relevant boxes in the checklist too, if addressed.
Please indicate how much time you'd like to finalize your review.
@rudeboybert great job at tending to the comments and cutting down the MS. I have two small remaining comments that if resolved, I think would mean the paper is good to go.
geom_categorical_model
: It's possible that the checkbox for "Functionality documentation" solely encompasses content in the README and software paper. If that's the case, you're fine, and I've actually just checked the box for that already. That said, for a potential user of this package, the function documentation for geom_categorical_model
still leaves room for confusion. For the general utility of this package, a short sentence about why some lines are dashed vs. solid would be nice.
Submitting author: @rudeboybert (Albert Y. Kim) Repository: https://github.com/moderndive/moderndive Version: v0.5.2 Editor: @moorepants Reviewer: @lisamr, @lwjohnst86 Archive: 10.5281/zenodo.5119261
Status
Status badge code:
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) by leaving comments 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
@lisamr & @lwjohnst86, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://openjournals.readthedocs.io/en/jose/reviewer_guidelines.html. Any questions/concerns please let @moorepants know.
✨ Please try and complete your review in the next four weeks ✨
Review checklist for @lisamr
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?Review checklist for @lwjohnst86
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?