-
-
Notifications
You must be signed in to change notification settings - Fork 36
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]: SpatialGEV: Fast Bayesian inference for spatial extreme value models in R #6878
Comments
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks. 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:
|
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: 🟡 License found: |
|
Review checklist for @fabian-sConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@meixichen could you try finding/adding the missing DOIs? (see above) |
Thanks @fabian-s for noting this. @meixichen please also try to reduce the length of the submission (following this pre-review-comment #6861 (comment)) |
Thanks for reviewing this manuscript and package. I will shorten the paper and add the missing DOIs. |
@editorialbot generate pdf |
@editorialbot check repository |
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: 🟡 License found: |
@editorialbot generate pdf |
@editorialbot check repository |
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: 🟡 License found: |
Hello, I wonder if 1494 word count is still considered too long? Thanks! |
Contribution and authorship: Seems like only @meixichen has a meaningful number of commits here -- what are the contributions of the other two authors? The only commit of @mlysy from 4 years ago (meixichen/SpatialGEV@47be58e) seems more like housekeeping to me and, in any case, git blame (https://github.com/meixichen/SpatialGEV/blame/master/inst/include/SpatialGEV/utils.hpp) seems to indicate that |
Automated tests: test coverage seems to be rather low, see meixichen/SpatialGEV#14 |
Community guidelines: you should add a CONTRIBUTING.md or similar to the repo, see https://contributing.md/example/ e.g. |
Functionality: |
Documentation: |
@meixichen any news re. my questions/requests above? |
@fabian-s Regarding meixichen/SpatialGEV#14 I have merged joss with master and added more unittests. Test coverage is close to 90% now https://app.codecov.io/github/meixichen/SpatialGEV/tree/master I am still working on the other requests and will reply here after I am done with them. For authorship, many conversations about functionalities and code design were done off-line or over emails. Some of mlysy's code were in the joss branch and were not reflected in master, but now after merging the joss into master we should see it. |
Hello @fabian-s , regarding the issues you flagged, I have made the following changes:
Any comments or suggestions? Thank you very much! |
Hi @fernandomayer just checking if there are any issues that blocks you to start with your review. |
Thanks, I closed the issue. The example with covariates in your vignette does not work: https://github.com/meixichen/SpatialGEV/blob/61466b5dde6e5f36965f2244fd8034224076cac3/vignettes/SpatialGEV-vignette.Rmd#L314C1-L340C1 is all just error messages...? If I re-compile it locally, it works fine, though: I don't really think it makes sense to include this vignette "pre-compiled" as you're doing now, it doesn't take that long to run and it's bad practice to do so anyway, to avoid exactly this kind of problem. You also provide the wrong paths to your image files in that pre-compiled fake vignette (the paths are `figures/bla.png', but the files are not in that subdirectory, it does not exist....) @vissarion not sure what your editorial standards are re. R packages. |
@fabian-s there are no editorial standard specifically for R packages. The general rule "packaged appropriately according to common community standards for the programming language being used", could mean that |
sure, I know, I also do editor-stuff for JOSS - that's why I said your standards ;) FWIW, the NOTES/WARNINGS/ERROS don't seem that serious to me, the package can be installed in the standard way and it runs, including examples etc. If it were my call, I would still request to make it CRAN-compliant s.t. the version of the package presented in the paper is more similar/identical to the respective CRAN version. |
I agree that it would be nice to make it CRAN-compliant. Though I would not like this to be a blocking issue. To my understanding there is not much work towards this goal (and in any case maybe that work will be done anyways to update the current CRAN package). @meixichen do you agree to make this package pass |
@fabian-s @vissarion I agree with you both. I have pushed the new changes on the vignette, which is now no longer pre-compiled. Passed the check for me:
|
for me as well |
@vissarion Sorry for the delay. I'll be starting my review this week, planning to finalise it mid-next week. |
Submitting author: @meixichen (Meixi Chen)
Repository: https://github.com/meixichen/SpatialGEV
Branch with paper.md (empty if default branch): joss
Version: v1.0.1
Editor: @vissarion
Reviewers: @fabian-s, @fernandomayer
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
@fabian-s & @fernandomayer, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review.
First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @vissarion 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 ✨
Checklists
📝 Checklist for @fabian-s
The text was updated successfully, but these errors were encountered: