Closed editorialbot closed 1 year ago
editorialbot
commented
1 year ago 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:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.31 s (588.9 files/s, 132773.3 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
JavaScript 16 3226 2940 12182
HTML 65 2062 205 7872
TeX 3 289 0 3640
R 64 421 1109 2340
Markdown 9 399 0 1122
XML 5 0 0 956
CSS 6 60 37 627
Rmd 6 229 320 394
Sass 1 4 0 71
YAML 3 3 4 61
SVG 1 0 1 11
JSON 1 0 0 1
-------------------------------------------------------------------------------
SUM: 180 6693 4616 29277
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 615
editorialbot
commented
1 year ago Failed to discover a valid open source license
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.5194/hess-23-2939-2019 is OK
MISSING DOIs
- None
INVALID DOIs
- https://doi.org/10.5281/zenodo.6350042 is INVALID because of 'https://doi.org/' prefix:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
crvernon
commented
1 year ago 👋 @mabesa , @mengqi-z, @tonyewong This is the review thread for the paper. All of our communications will happen here from now on.
Please read the "Reviewer instructions & questions" in the first comment above.
Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/4805 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.
mengqi-z
commented
1 year ago 
tonyewong
commented
1 year ago tonyewong
commented
1 year ago I see that EditorialBot found this as well, but I opened an issue in the repo, regarding an open source license (https://github.com/hydrosolutions/riversCentralAsia/issues/97). Can one be added to the top level of the repo and a short section in the top level README.md that summarizes license info?
mabesa
commented
1 year ago Thank you for pointing that out. I added a license file and added a section in the README.
tonyewong
commented
1 year ago I don't know if it needs addressing, but just FYI - as I installed, the dependency on package "exactextractr" threw an error message. I had to install it separately in a first step, and say "no" when the installer asks "Do you want to install from sources the package which needs compilation? (Yes/no/cancel)". But once I did that, things installed and the example in the top-level README runs just fine.
tonyewong
commented
1 year ago @mabesa The example in the README works nicely out-of-box, but I'm having issues running the tests in riversCentralAsia/tests/testthat.R/tests. Specifically, I get:
Error in `test_dir()`:
! No test files found
Run `rlang::last_error()` to see where the error occurred.Can some additional guidance on running those tests be provided in the README? (or elsewhere)
mabesa
commented
1 year ago @tonyewong I also had problems with exactextractr very recently, it started returning wrong results after the last update. I think I should probably immediately have this documented here... I'm sorry, this is the first time I run through a software review. Thank you for noting the installation problem, I hadn't seen that one yet! I will try to solve this issue and let you know asap.
I devtools::test() runs without error in the main directory. Also, I can navigate to ./tests and run testthat.R without problems or run tests of individual functions without error messages. Could you please specify how you ran the tests so I can reproduce the error message?
crvernon
commented
1 year ago 📣 Mid-week rally!
👋 @tonyewong - It looks like you and @mabesa are working a few things out but all is going well. Let me know if you need anything.
👋 @mengqi-z - Can you provide a status update to how things are going on your side of things? Thanks!
Keep up the good work!
mabesa
commented
1 year ago Yep, I updated the package and tested it with the devtools utils and I am now re-testing the package with real-life data sets not included in the test suite. This procedure should be done tomorrow morning by the latest. I'll write again then, hopefully with an "issue fixed" happy-face emoji.
crvernon
commented
1 year ago Great, thanks @mabesa !
mengqi-z
commented
1 year ago @crvernon - I am planning to get more reviews done by the end of this week.
@mabesa - a quick note for the installation step. The built-in pip operator "|>" used in the package requires R version >= 4.1. Could you add this requirement to the installation instruction? Thanks.
tonyewong
commented
1 year ago @mabesa - Edit: I moved this discussion of the error I saw while running the test functions to an issue in the repo (https://github.com/hydrosolutions/riversCentralAsia/issues/100)
mabesa
commented
1 year ago @mengqi-z - Thank you, I added the R version to the README. I will write once the test issues are solved.
mabesa
commented
1 year ago Test issues are resolved
mabesa
commented
1 year ago I'm working on an issue deploying the package (the package maintainer needs to remove an old workflow which does not seem to be working any more). Gosh, suddenly so many issues! But they can be solved step by step.
I will be on a family holiday most of next week and have promised not to bring the laptop. I'm sorry if this interferes with the review process.
tonyewong
commented
1 year ago I fully support the no-laptop family holiday rules!
mengqi-z
commented
1 year ago Hi @mabesa - one more data point following the issue tonyewong mentioned previously, I also need to manually install package exactextractr before I can installed riverCentralAsia successfully. In addition, I posted an issue specifically for issues I encountered when going through vignettes https://github.com/hydrosolutions/riversCentralAsia/issues/109. Thanks!
mabesa
commented
1 year ago I'm back Thank you so much @mengqi-z for taking the pains to go through the vignette articles in such detail. You input is very useful! I'll address them asap.
mabesa
commented
1 year ago About the problem with the automatic installation of the exactextractr package: @mengqi-z and @tonyewong, could you please let me know: do you work on windows or another system? Was there a specific error message related to the exactextractr package? It always installs without problem on my windows machine. Maybe this is something I should take up with the developer.
mabesa
commented
1 year ago The package site is now deployed correctly and the points in the issues addressed. Thank you again both reviewers for your input!!
mabesa
commented
1 year ago A colleague of mine also kindly tested the clean-slate installation and didn't have issues with exactectractr. We also didn't encounter installation problems (other than missing RTools) with our students. I'm a bit stumped.
mengqi-z
commented
1 year ago Hi @mabesa,
I am using MacOS Monterey. I think it might be the compilation issue, because I can install it separately when choosing to install from binary version (0.7.2), instead of the source version (0.9.0) which need compilation. In the latter case, geos-config does not exist and causes the problem. Here is the error message I got for the part where exactextractr failed to install during riverCentralAsia installation in the case of choosing "Yes" for the question "Do you want to install from sources the package which needs compilation? (Yes/no/cancel)":
* installing *source* package ‘exactextractr’ ...
** package ‘exactextractr’ successfully unpacked and MD5 sums checked
** using staged installation
configure: exactextractr: 0.9.0
checking for geos-config... no
configure: error: geos-config not found or not executable
ERROR: configuration failed for package ‘exactextractr’
* removing ‘/Library/Frameworks/R.framework/Versions/4.2-arm64/Resources/library/exactextractr’
The downloaded source packages are in
‘/private/var/folders/0f/xg802qr91dd8gmmk8vwgsz340000gn/T/RtmpbGfVD1/downloaded_packages’
✔ checking for file ‘/private/var/folders/0f/xg802qr91dd8gmmk8vwgsz340000gn/T/RtmpbGfVD1/remotes173f7c639586/hydrosolutions-riversCentralAsia-5ee0d64/DESCRIPTION’ ...
─ preparing ‘riversCentralAsia’:
✔ checking DESCRIPTION meta-information ...
─ checking for LF line-endings in source and make files and shell scripts
─ checking for empty or unneeded directories
─ building ‘riversCentralAsia_1.1.0.tar.gz’
ERROR: dependency ‘exactextractr’ is not available for package ‘riversCentralAsia’
* removing ‘/Library/Frameworks/R.framework/Versions/4.2-arm64/Resources/library/riversCentralAsia’
Warning messages:
1: In i.p(...) :
installation of package ‘exactextractr’ had non-zero exit status
2: In i.p(...) :
installation of package ‘/var/folders/0f/xg802qr91dd8gmmk8vwgsz340000gn/T//RtmpbGfVD1/file173f66f10b30/riversCentralAsia_1.1.0.tar.gz’ had non-zero exit statusI am also using MacOS Monterey.
mabesa
commented
1 year ago Thanks a lot! It is a Mac issue then (My colleague and me are working on Windows 11 at the moment). I have made a note in readme and written to the developer of exactextractr. I'll organize a mac for testing too and possibly should run a test on ubuntu as well...
tonyewong
commented
1 year ago @mabesa Could a bit more be said in the Summary section of the paper about how this package might be useful in some broader applications, beyond specifically for students? I can appreciate that much of this was developed for a course, but I can also see how this sort of software will be useful for practitioners and researchers, in addition to students.
Then in Statement of Need/Related packages, it isn't immediately clear to me as a non-specialist how RS Minerve and the other mentioned packages fits into this picture in terms of functionality, as compared to the riversCentralAsia package. I was maybe a little confused here because the first part of Related Packages mentions the RSMinverveR package, which it says can run RS Minerve through R. But later this section says "there is no R package to facilitate hydrological modelling specifically with RS Minerve". There is probably a distinction here that I'm not quite getting. Can this part be clarified?
Edit: Can the Summary also be expanded slightly to mention that this software takes care of processing many standard data types? I was poking through the documentation site (which is impressive), which makes this clear, particularly on the "Description of data raw types" page. The paper's Summary section could also serve to advertise this feature.
I'll open an issue in the repository for these, but wanted to mention it here first as well.
I think these points would wrap up the documentation portion of my review. Then I just need to finish running through a few more of the examples in the documentation, and I should be all set soon!
mabesa
commented
1 year ago I can't get the .github/workflows/draft-pdf.yml workflow to compile the current version of the paper branch. I can only re-run the last compilation which was sometime in October. paper.md is updated but I'm not sure the paper pdf is beeing updated for joss as it should be.
tonyewong
commented
1 year ago I'm curious if us mortals are allowed to do this, and if so, if it helps at all with getting the pdf updated... :
tonyewong
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mabesa
commented
1 year ago Oh, thanks!! Now I need to find a fix for the image which is not being rendered
mabesa
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mabesa
commented
1 year ago @tonyewong thanks for the hint with the editorialbot command. The figure now shows. So: I did a (admittedly very minimalistic) update of the paper text by adding a very rough description of the hydrological modelling workflow where the package is used. Please let me know if this makes the package purpose more understandable or if it adds to the confusion. I could free up some time next week to restructure the paper text.
tonyewong
commented
1 year ago Looks good! The one point that I think might still be a bit unclear is the relationship to the RSMinverveR package:
Then in Statement of Need/Related packages, it isn't immediately clear to me as a non-specialist how RS Minerve and the other mentioned packages fits into this picture in terms of functionality, as compared to the riversCentralAsia package. I was maybe a little confused here because the first part of Related Packages mentions the RSMinverveR package, which it says can run RS Minerve through R. But later this section says "there is no R package to facilitate hydrological modelling specifically with RS Minerve". There is probably a distinction here that I'm not quite getting. Can this part be clarified?
mabesa
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mabesa
commented
1 year ago @tonyewong done!
tonyewong
commented
1 year ago Thanks for making those edits! I think it's still a bit unclear what sets riversCentralAsia apart from RSMinerveR - what is the distinction, or what additional functionality/ease of use/etc. does riversCentralAsia provide that RSMinerveR does not?
mabesa
commented
1 year ago I'm finally starting to see your point. RSMinerveR is not a real software package. It includes some of the functions most of which I copied to riversCentralAsia later so they are in fact redundant. The essential part in RSMinerveR are the examples/vignettes that demonstrate how to use the CLR interface to run RS MINERVE from R without using the GUI of RS MINERVE. I'll rephrase the sections to clarify that and I'll clean out redundancies from RSMinerveR as soon as I have time!
mabesa
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mabesa
commented
1 year ago both riversCentralAsia and RSMinerveR are update. RSMinerveR is still organised as a package but it now only contains the examples as vignettes, no redundant R functions. The examples use the functions from riversCentralAsia.
tonyewong
commented
1 year ago Great! That edit in the paper makes the distinction much more clear. Thank you!
Then I just have a couple more of the examples in the documentation to finish running through and I'll be done.
Submitting author: !--author-handle-->@mabesa<!--end-author-handle-- (Beatrice Marti) Repository: https://github.com/hydrosolutions/riversCentralAsia/ Branch with paper.md (empty if default branch): paper Version: v1.2.0 Editor: !--editor-->@crvernon<!--end-editor-- Reviewers: @mengqi-z, @tonyewong Archive: 10.5281/zenodo.7583428
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
@mengqi-z & @tonyewong, 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 @crvernon 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 @mengqi-z
📝 Checklist for @tonyewong