Skip to content
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]: ggalluvial: Layered Grammar for Alluvial Plots #2017

Open
whedon opened this issue Jan 16, 2020 · 53 comments
Open

[REVIEW]: ggalluvial: Layered Grammar for Alluvial Plots #2017

whedon opened this issue Jan 16, 2020 · 53 comments
Assignees
Labels

Comments

@whedon
Copy link
Collaborator

@whedon whedon commented Jan 16, 2020

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

Status badge code:

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

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:

  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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @akeshavan know.

Please try and complete your review in the next two weeks

Review checklist for @clauswilke

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

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?
  • Contribution and authorship: Has the submitting author (@corybrunson) 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 functionality 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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @njtierney

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

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?
  • Contribution and authorship: Has the submitting author (@corybrunson) 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 functionality 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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jan 16, 2020

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 🎉.

⭐️ 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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jan 16, 2020

Reference check summary:

OK DOIs

- 10.1007/978-4-431-68057-4_3 is OK
- 10.1080/01621459.1990.10474926 is OK
- 10.1198/106186002317375631 is OK
- 10.1109/INFVIS.2005.1532152 is OK
- 10.1007/3-540-28084-7_4 is OK
- 10.1007/0-387-28695-0 is OK
- 10.1109/TVCG.2006.76 is OK
- 10.1111/j.1530-9290.2008.00004.x is OK
- 10.1198/jcgs.2009.07098 is OK
- 10.1371/journal.pone.0008694 is OK
- 10.1109/TVCG.2011.227 is OK
- 10.1007/978-3-642-21551-3_13 is OK
- 10.1068/a45488 is OK
- 10.1109/TVCG.2013.140 is OK
- 10.18637/jss.v059.i10 is OK
- 10.1038/srep06773 is OK
- 10.1007/978-0-387-98141-3 is OK
- 10.1177/0308518X17745067 is OK
- 10.1016/j.anaerobe.2018.05.017 is OK
- 10.21105/joss.01686 is OK
- 10.31234/OSF.IO/MKQN4 is OK
- 10.1002/ejhf.1547 is OK
- 10.1111/gec3.12441 is OK
- 10.1523/jneurosci.1451-18.2019 is OK
- 10.3389/fimmu.2019.00660 is OK
- 10.1097/CCM.0000000000003469 is OK
- 10.1016/j.marpol.2019.103676 is OK
- 10.1016/j.eiar.2019.01.003 is OK
- 10.1016/j.fcr.2019.03.023 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jan 16, 2020

@clauswilke

This comment has been minimized.

Copy link
Collaborator

@clauswilke clauswilke commented Jan 17, 2020

Overall this looks good to me. I have a few minor comments:

  • Consistent use of lower-case "i" looks strange to me. I think the writing should follow the standard English convention of using upper-case "I" when referring to oneself.
  • The first example in the (README)[https://github.com/corybrunson/ggalluvial/blob/master/README.md] appears to use a deprecated parameter.
  • I would like to ask the author to put a little more effort into comparing this package to other approaches. In particular, ggforce has a very mature parallel sets implementation, and it is not clear to me how the ggforce features are similar or different from the ggalluvial features. This would benefit from a little more exposition. Also, while I understand that Sankey plots are very different from the plots generated by ggalluvial, I don't understand the difference between the alluvial plots and parallel sets. Is it just the gaps between groupings? In this context, I'd like to point out that ggforce puts gaps between groupings, but those gaps can be disabled as far as I remember.

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.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Jan 17, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jan 17, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Jan 17, 2020

@clauswilke thank you for this feedback. I think i've addressed most of your concerns:

  • In order to avoid undermining the change in later revisions, i'll do a find--replace from "i" to "I" once the paper is in its final form.
  • I have corrected the outdated use of the label.strata parameter. Thank you for catching it!
  • I have revised the "Concepts" section with a more careful and detailed comparison, in which i've tried to describe the ways ggalluvial is different from the other parallel sets packages in order to justify the classification scheme (Sankey, parallel sets, alluvial) i use but without suggesting that any is generally superior to the others. I think this addresses your third point; does it also address your fourth? I very much agree with you about the value of having multiple packages that accomplish similar but distinct things, but i do think one contrast in particular (allowing versus disallowing gaps and the meaningfulness of cumulative axis) is more significant than users tend to recognize, and which i think comes through most clearly in the third application, so i've made that more explicit.
@clauswilke

This comment has been minimized.

Copy link
Collaborator

@clauswilke clauswilke commented Jan 17, 2020

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.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Jan 18, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jan 18, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Jan 18, 2020

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

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Feb 10, 2020

@corybrunson - it's now been about 3 weeks - can you update us all on the status of this submission/review?

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Feb 10, 2020

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

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Feb 10, 2020

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

@akeshavan

This comment has been minimized.

Copy link

@akeshavan akeshavan commented Feb 10, 2020

👋 @njtierney - have you had a chance to take a look at this package?

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

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Feb 10, 2020

@akeshavan it would be a minor version increment, but, regardless, i don't mind waiting. Thanks!

@akeshavan

This comment has been minimized.

Copy link

@akeshavan akeshavan commented Feb 21, 2020

👋 hi @kellieotto and @tweed1e -- would you be interested in reviewing this paper for JOSS?

@kellieotto

This comment has been minimized.

Copy link
Collaborator

@kellieotto kellieotto commented Feb 25, 2020

@akeshavan Sorry, I don't have the bandwidth right now.

@tweed1e

This comment has been minimized.

Copy link
Collaborator

@tweed1e tweed1e commented Feb 25, 2020

@akeshavan I'll have time starting this weekend, happy to help 👍

@akeshavan

This comment has been minimized.

Copy link

@akeshavan akeshavan commented Feb 28, 2020

@whedon add @tweed1e as reviewer

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Feb 28, 2020

OK, @tweed1e is now a reviewer

@tweed1e

This comment has been minimized.

Copy link
Collaborator

@tweed1e tweed1e commented Mar 3, 2020

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:

  • Make an alluvial diagram (maybe just re-use the Titanic one) with labels pointing to an alluvium, a lode, a flow, a strata. I thought that's what the diagram on page 2 was going to be, so I got extra confused when a stratum has a 'flow' label, etc etc.
  • For the plot on page 2: add a label (e.g., Figure 1: Alluvial plot relating geoms and stats) and reference that label in the text (e.g., "The alluvial plot in Figure 1" instead of "the following alluvial plot")

Then I'll check off "Summary" and "Quality of writing" and accept 👍.

@clauswilke

This comment has been minimized.

Copy link
Collaborator

@clauswilke clauswilke commented Mar 3, 2020

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.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 3, 2020

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

@tweed1e

This comment has been minimized.

Copy link
Collaborator

@tweed1e tweed1e commented Mar 4, 2020

Yea, that should help non-specialists like me understand it. Go for it 👍👍

@arfon

This comment has been minimized.

Copy link
Member

@arfon arfon commented Mar 14, 2020

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.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 16, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Mar 16, 2020

@clauswilke

This comment has been minimized.

Copy link
Collaborator

@clauswilke clauswilke commented Mar 16, 2020

I am fine with this revised manuscript. No more comments from me.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 16, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Mar 16, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 16, 2020

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

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 22, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Mar 22, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 22, 2020

@tweed1e i learned how to use ggplot2::annotate() and added labels and arrows to the plot! I think this is what you had in mind.

@tweed1e

This comment has been minimized.

Copy link
Collaborator

@tweed1e tweed1e commented Mar 22, 2020

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! 👍

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 22, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Mar 22, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 22, 2020

@tweed1e done! Thank you for those valuable suggestions.

@tweed1e

This comment has been minimized.

Copy link
Collaborator

@tweed1e tweed1e commented Mar 22, 2020

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.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 22, 2020

@tweed1e thank you. : ) @akeshavan no urgency. I very much appreciate your efforts.

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 24, 2020

@akeshavan i wonder if it's too late for me to move the file paper.md into the paper subdirectory. This would clean up the main directory a bit. But i gather it would affect whedon's ability to generate the PDF.

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Mar 24, 2020

@corybrunson - that should be fine to do

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 24, 2020

@danielskatz thank you—i moved the files and pushed the commit, but i didn't make any changes to the paper itself.

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Mar 24, 2020

You can always test the build, like I am now going to do

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Mar 24, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Mar 24, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 24, 2020

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Mar 24, 2020

@corybrunson

This comment has been minimized.

Copy link
Collaborator

@corybrunson corybrunson commented Mar 24, 2020

Thanks! It looks good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.