-
Notifications
You must be signed in to change notification settings - Fork 0
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
Draft package vignettes #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cforgaci,
the vignettes look nice and give a clear impression of what CRiSp does/will do. I've added a number of minor comments, largely addressing very minor issues or where I hope they can add some additional clarity. None of these are crucial, however, so please do with them as you see fit.
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cforgaci , looks great, it all reads very clearly and it looks to me very useful as a general guideline to structure the package!
I like the separation between methodological and use-case-driven vignettes. Feel free to merge on my side.
As a general comment, I agree with your suggestion of replacing API calls/preprocessing steps with packaged data in most of the vignettes. We could maybe think about moving all OSM-related stuff to a separate vignette that focuses on how to get data from OSM in a form that is suitable to work with CRiSp - basically documentation on how to use OSM-related utility functions of CRiSp.
All other vignettes could then start from packaged data that is already cleaned and preprocessed, so that it will be clearer to the reader what are the main core functions of CRiSP (i.e. the delineation methods). This would also force us to expose and document functions that run each of the delineation steps independently and using arbitrary datasets in input (say, a river line, a street network, etc.), in addition to the all-at-once functions that automatically get data from OSM (such as delineate_corridor
). But I think this is a good thing, since not everyone will be working (exclusively) with OSM data.
Thanks @meiertgrootes! I tried to address all the comments. |
Perfect, thanks for the review!
I created #22 for this.
This PR is ready to be merged. |
I drafted a set of vignettes which are meant to explain to the end user how the package works.
The vignettes are organised in two categories:
The distinction between these two categories is also indicated on the package website.
Note that:
Vignettes ready for review:
Delineation:
Use cases:
I hope this helps us sharpen our understanding of the package functionality and design.