-
-
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]: stemflow: A Python Package for Adaptive Spatio-Temporal Exploratory Model #6158
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:
|
|
Wordcount for |
|
Hi @jedbrown & @earth-chris - happy new year! As you're ready, the review for this contribution will take place in this issue thread. The reviewer guidelines are available here. Thank you for your time! |
Hi 👋 looks like you meant to tag @jedalong. |
Review checklist for @earth-chrisConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Hi @chenyangkang, I have concluded my review - thank you for the contribution! This is a valuable software package. This software passes the majority of the checklist-based review guidelines, and the majority of my comments are related to improved documentation regarding the functionality of the package (see here). I also created new issues regarding automated testing and community guidelines. I don't believe any of the above issues merit withholding publication, and would be willing to check the boxes if @diazrenata is sufficiently satisfied (there are manual, not automated tests; a statement on contribution exists, but is minimal). But I at least wanted to flag them as minor issues. |
@jedbrown My mistake! I apologize for the confusion. |
Hi @earth-chris, thank you for the comments on Based on your suggestions, I did the following revisions:
The details could be found in the issue. I also added automated testing and contributor guidelines. Again I want to thank you for reviewing |
Hi @chenyangkang Here are my comments on the paper hopefully they are useful. I couldnt figure out how to do the checklist. I think the package is useful and the documentation is well delivered, but I have one comment about how the spatial binning is organized that you should at least think about in detail. L10 - I think you need to explain what AdaSTEM is more explicitly at the beginning of the abstract similar to line 32 so the reader knows what this is. My main issue with the structure of the code is the inability to leverage the existing spatial infrastructure in python. Using only tabular data and then using a cartesian gridding process is inadequate for global data (on a sphereical earth). The data is assumed to be in a cartesian plane (which latitude and longitude are not) and then divided into 'grids'. In the mallard example you use 50 spatial and temporal blocks for the global distribution of mallards. One degree of longitude is always about 110km so 7.2 km is about 792 km. The big issue is that 1 degree of latitude varies from 110km at the equator to 0 km at the poles, so the area of your blocks varies greatly from equator to pole. Gridding data on the globe without accounting for the spherical geometry of the earth is problematic (your package is not alone in ignoring this major issue). Could you instead allow the users to pass in spatial objects or define bins based on actual geometry so that bins are more equally sized to reflect global distribution data. To me it would be more approparite for a user to pass in a single parameter associated with the desired output spatial resolution of the grid size (e.g., grids with a size of 100km x 100km) and then the package would create the grid on the fly from a single parameter. You expecte 4 parameters for this same gridding process? I note that your package s does not force grid cells to be square in area, which is IMO unusual. This issue is further demonstrated in the Tips section for using a different coordinate system where the user must pass in 1000 to 10000 m as the range for "latitude" and "longitude" values in another coordinate system that does not use latidtude and longitude but rather x and y. I think it is probably associated with the tools used for modelling that do not expect spatial objects, but binning the data correctly based on spatial objects and then back transforming to the expected data structure migth be a more correct way to deal with the spherical geometry issue. What is the difference between temporal_step and temporal_bin_interval? I didnt follow the whole 'sliding' window part of this are they bins/blocks or a moving window? Documentation Notes: |
Hi @jedalong, thank you for your review! To get your checklist working, could you try adding a comment to this thread with this text (and this text only)?
|
Hi @jedalong, thank you for the valuable suggestions! I agree with most of your suggestions, and here are my responses:
Again, thank you for your suggestions and contribution to |
Review checklist for @jedalongConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
hi @chenyangkang and @diazrenata, sorry for the delay but I'm just writing to confirm that I have checked the remaining boxes on my review. It looks like |
@editorialbot assign me as editor I'm going to take over as editor! Thanks for your review @earth-chris! @jedalong I see your review list is checked off and the author opened some issues related to your comments which they have also subsequently closed. Can you look through the comments and issues to verify if your concerns have been addressed, and if you consider your review finished? |
Assigned! @kthyng is now the editor |
Post-Review Checklist for Editor and AuthorsAdditional Author Tasks After Review is Complete
Editor Tasks Prior to Acceptance
|
@chenyangkang Please go through the bullet list above (you might copy-paste it so you can use it as a checklist) and let me know when you're finished with the actions. |
|
Hi @kthyng, I have completed the checklist. The new version is Thanks! |
@editorialbot set v1.1 as version |
Done! version is now v1.1 |
@editorialbot set 10.6084/m9.figshare.24669132 as archive |
Done! archive is now 10.6084/m9.figshare.24669132 |
@editorialbot generate pdf |
@chenyangkang paper comments:
|
@editorialbot check references |
|
Hi @kthyng, typo fixed! |
@editorialbot generate pdf |
@editorialbot accept |
|
Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository. If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file. You can copy the contents for your CITATION.cff file here: CITATION.cff
If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation. |
🐘🐘🐘 👉 Toot 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... |
Congrats on your new publication @chenyangkang! Many thanks to early editor @diazrenata and to reviewers @jedalong @earth-chris for your time, hard work, and expertise!! |
🎉🎉🎉 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:
|
@kthyng @diazrenata thank you so much for your editorial effort and devotion to this great journal! ❤️ @earth-chris @jedalong Thank you for your valuable suggestions that make stemflow significantly better and more meaningful to the community. I deeply appreciate your time and advice! Wish you all the best! Yangkang |
Thank you @chenyangkang! If you are interested in contributing to JOSS, please consider signing up as a reviewer! https://reviewers.joss.theoj.org/](https://reviewers.joss.theoj.org/) |
Thanks @kthyng! I already did! Looking forward to contributing! |
Submitting author: @chenyangkang (Yangkang Chen)
Repository: https://github.com/chenyangkang/stemflow
Branch with paper.md (empty if default branch): main
Version: v1.1
Editor: @kthyng
Reviewers: @jedalong, @earth-chris
Archive: 10.6084/m9.figshare.24669132
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
@jedalong & @earth-chris, 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 @diazrenata 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 @earth-chris
📝 Checklist for @jedalong
The text was updated successfully, but these errors were encountered: