-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: rmap: An R package to plot and compare tabular data on customizable maps across scenarios and time #4015
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @CamilleMorlighem, @maczokni it looks like you're currently assigned to review this paper 🎉. 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. ⭐ 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:
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:
|
Wordcount for |
|
|
Hello ! I started the review. I have a few comments already:
|
Thanks for the very useful feedback and comments. Here are the responses to your points:
|
Hi @zarrarkhan, Thank you for your responses ! I have tested the documentation and functionality of the package. The documentation is very clear and structured and very user friendly! I just have the following comments for improvement:
About the functionality, everything worked fine and I don't have any performance claims. I only have the following comments:
Feel free to ignore any comment if you think that's not relevant! |
👋 @maczokni, please update us on how your review is going (this is an automated reminder). |
👋 @CamilleMorlighem, please update us on how your review is going (this is an automated reminder). |
@CamilleMorlighem Thanks for the detailed review and very helpful comments. Addressed as follows: Improvements
Functionality
|
Hello! I've hit a bit of a stumbling block when trying to follow along with some data that isn't the one in the examples (I understand the idea to be that users can upload their own .csv files and this package will allow them to easily map this?) First note: if the column is not called "subRegion" then the error But where I'm stuck: once I try to map my data, it will not do this. I have renamed to values, and have checked and it is numeric. Here's my code
Any thoughts? |
Hi @maczokni. Thanks for your review and comments. Please see our responses below:
Please let us know if this helps clarify some of the issues you were having and if you have additional concerns/comments. |
Hi @zarrarkhan thanks for the update - sorry just getting back to this now but I have a major concern - you are right there are duplicates, as there are USA counties which share the same name. This means that the county names is not actually an appropriate column on which to join. This means that if I do want to join the full dataset (rather than the 10 subset you used) then I cannot do this with the package - is that right? And this is not just a quirk of the dataset I'm using to test, your own data (the inbuilt map: mapUS49County has duplicate values of the subRegion column):
Considering this is a primary function of the package, do you think you could take some more time to think about this problem? As I'm sure you are familiar, usually for each geography you will look for unique codes in the dataframes on which you can confidently join. Here in the UK we might use for example local authority codes or output area codes. In the US it seems to be state and county codes for example in this case, which is in the ncovr dataset as 'FIPS'. I had a play with the data of the mapUS49County inbuilt map as an example, and by pasting the state and county code you can create a FIPS column to join.
When we do that only 3 counties are not joined - i.e. only 3 would not be linked to the inbuilt map. You might want to consider printing a warning to users when this is the case, maybe with the name of the ones which did not join. However this is only one case for one data set - I think it might be useful to think about some use cases of external data, how do these codes commonly look like, and have a few join columns on which users' data can be joined to the inbuilt geometries. Hope this makes sense, let me know if not I can try to elaborate more! |
@maczokni
(Source: https://www.ddorn.net/data/FIPS_County_Code_Changes.pdf)
Please let us know if this addresses some of your concerns or if you have additional comments. |
@CamilleMorlighem I see that the authors fixed your issues (statement of needs among others, see there: #4015 (comment)) but you didn't tick the boxes above. Can you let us know if you are now satisfied please? |
@hugoledoux thank you for noticing it ! @zarrarkhan thank you for these improvements and answers to my comments. I think I'm done with the review. Rmap is a very nice package! I'm probably gonna use it in the future. |
@hugoledoux Thanks for following up. Wondering if there are any additional comments we can address or get an update from the reviewers on the comments we addressed in #4015 (comment). |
@maczokni could you check the updates from the authors (#4015 (comment))? And tell us if you're satisfied? |
@danielskatz |
should be - let's see :) |
@editorialbot recommend-accept |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3534, then you can now move forward with accepting the submission by compiling again with the command |
|
sorry @zarrarkhan - there is one more change needed - for the first Pebesma reference, please add |
no worries! Done and merged into main. |
@editorialbot recommend-accept |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3535, then you can now move forward with accepting the submission by compiling again with the command |
|
@editorialbot accept |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congratulations to @zarrarkhan (Zarrar Khan) ad co-authors!! And thanks to @CamilleMorlighem and @maczokni for reviewing, and to @hugoledoux for editing! |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! The 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:
|
@danielskatz @hugoledoux @maczokni @CamilleMorlighem |
Submitting author: @zarrarkhan (Zarrar Khan)
Repository: https://github.com/JGCRI/rmap
Branch with paper.md (empty if default branch):
Version: v1.0.5
Editor: @hugoledoux
Reviewers: @CamilleMorlighem, @maczokni
Archive: 10.5281/zenodo.7085969
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.
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
@CamilleMorlighem & @maczokni, 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 @hugoledoux know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Review checklist for @CamilleMorlighem
✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @maczokni
✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: