Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[REVIEW]: ggalluvial: Layered Grammar for Alluvial Plots #2017
Comments
This comment has been minimized.
This comment has been minimized.
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @clauswilke, @njtierney it looks like you're currently assigned to review this paper 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Overall this looks good to me. I have a few minor comments:
Finally, in the comparison to other packages, I don't think it's so important to create a contrast between the present work and other works. This feels forced in places. It's useful to have multiple high-quality implementations of similar concepts, and there is no need to create an artificial contrast or show that one is better than the other. Just state what ggalluvial does and doesn't do, and what other packages do and don't do. |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@clauswilke thank you for this feedback. I think i've addressed most of your concerns:
|
This comment has been minimized.
This comment has been minimized.
I am fine with the revision. One last comment: The "Concepts" section needs some copy-editing. In places it is difficult to follow the prose. For example, by the time I reach the sentence "... the plots generated by these extensions ..." I have completely lost track of what "these extensions" refers to. You may benefit from talking through this section with a colleague, just so you get third-party feedback on whether the text flow can be followed or not. |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@clauswilke thank you again—i've made some more revisions that i'll run by a colleague and may revise further once the workweek resumes. |
This comment has been minimized.
This comment has been minimized.
@corybrunson - it's now been about 3 weeks - can you update us all on the status of this submission/review? |
This comment has been minimized.
This comment has been minimized.
@danielskatz thanks for the prompt. I believe i've addressed the first reviewer's concerns, and i should be able to address any from the second reviewer within a few days' time. (I'm traveling but will only be a few days at a time without wifi.) Incidentally, i've almost finished an upgrade on a new branch that i didn't think i'd be able to get to for some time (alternative curves for plotting flows; i mention this in the paper as the only planned feature not yet implemented). If the review is still open, would it be OK for me to merge this branch in? It would increment the version number. |
This comment has been minimized.
This comment has been minimized.
@corybrunson - the editor is @akeshavan who should be able to answer your question - I'm the Associated Editor in Chief on duty this week, and part of my job is to check on anything stale, and get it moving again. |
This comment has been minimized.
This comment has been minimized.
@corybrunson I think I'd wait to merge if it is a major upgrade, because one reviewer has already reviewed the current version's functionality. |
This comment has been minimized.
This comment has been minimized.
@akeshavan it would be a minor version increment, but, regardless, i don't mind waiting. Thanks! |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@akeshavan Sorry, I don't have the bandwidth right now. |
This comment has been minimized.
This comment has been minimized.
@akeshavan I'll have time starting this weekend, happy to help |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
OK, @tweed1e is now a reviewer |
This comment has been minimized.
This comment has been minimized.
I second the copy-editing request of the other reviewer, and my last hiccup: I get really confused by the terminology. I know about ggplot2 and its extensions, I know a bit about sankey diagrams and similar things, so I think I'm kind of the non-specialist audience that would need to understand the paper / documentation. I get the need for specific terminology to nail down exactly what the terms mean so people can understand the differences between a parallel sets, alluvial, and sankeys, but in combo with the prose (see @clauswilke's comment above), it's hard for the general audience (non-geologists, I guess?) to grasp what the purpose is and what the individual parts are without doing a deep dive into viz generally and the package itself. I'll make a few specific requests that I think will help:
Then I'll check off "Summary" and "Quality of writing" and accept |
This comment has been minimized.
This comment has been minimized.
Since I also haven't checked "quality of writing": I'm mostly waiting to look at the final version. I think the comments by @tweed1e are helpful, and I expect that I can check off "quality of writing" once they have been addressed. |
This comment has been minimized.
This comment has been minimized.
@tweed1e thank you! To address your first request, do you think it would suffice to add a lightly edited version of the "Alluvial plots" section of the primary vignette to the JOSS paper? I originally erred toward concision but i certainly don't mind adding useful content to the paper. If that makes sense, then i'll also add figure labels, then push the revised paper and have whedon re-generate the PDF. |
This comment has been minimized.
This comment has been minimized.
Yea, that should help non-specialists like me understand it. Go for it |
This comment has been minimized.
This comment has been minimized.
Dear authors and reviewers We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required. Thanks in advance for your understanding. Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team. |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I am fine with this revised manuscript. No more comments from me. |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@clauswilke thank you! @tweed1e i've addressed some of your concerns with the new example, adapted from a data set that comes with the package. I did away with the alluvial plot of geom–stat pairings, since it was contrived in the first place to provide a plot for the paper. I can add labels and pointers if it would add to the quick-readability, but i wanted to first push a version that's reproducible from code. |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@tweed1e i learned how to use |
This comment has been minimized.
This comment has been minimized.
Yep, that looks better. Still a bit confused but the labels are helping---e.g., it seems the alluvia and flows are the same until I notice the label "one alluvium", and that helps me tell the difference. Last request: remove the "y" axis label, and make the x-axis labels consistent with the text---the text says "Term", but the x-axis tick labels say "CURR1", etc. So either change "CURR1" to "Term 1", or add "Term" as the x-axis label, and 1, 3, 5, as the x-axis tick labels. Otherwise it should be good to go! |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@tweed1e done! Thank you for those valuable suggestions. |
This comment has been minimized.
This comment has been minimized.
Thanks! @akeshavan it looks like everything's good for us reviewers. @corybrunson I'm guessing you might have to wait a bit for this to get moving because of you-know-what. |
This comment has been minimized.
This comment has been minimized.
@tweed1e thank you. : ) @akeshavan no urgency. I very much appreciate your efforts. |
This comment has been minimized.
This comment has been minimized.
@akeshavan i wonder if it's too late for me to move the file |
This comment has been minimized.
This comment has been minimized.
@corybrunson - that should be fine to do |
This comment has been minimized.
This comment has been minimized.
@danielskatz thank you—i moved the files and pushed the commit, but i didn't make any changes to the paper itself. |
This comment has been minimized.
This comment has been minimized.
You can always test the build, like I am now going to do |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks! It looks good now. |
whedon commentedJan 16, 2020
•
edited by clauswilke
Submitting author: @corybrunson (Jason Cory Brunson)
Repository: https://github.com/corybrunson/ggalluvial
Version: v0.11.1
Editor: @akeshavan
Reviewers: @clauswilke, @njtierney, @tweed1e
Archive: Pending
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
@clauswilke & @njtierney, 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://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @akeshavan know.
Review checklist for @clauswilke
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @njtierney
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper