Skip to content
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

Merged
merged 39 commits into from
Aug 31, 2024
Merged

Draft package vignettes #17

merged 39 commits into from
Aug 31, 2024

Conversation

cforgaci
Copy link
Contributor

@cforgaci cforgaci commented Aug 26, 2024

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:

  1. Vignettes for each step of the delineation process: corridor edge delineation, segmentation, and river space delineation. The first vignette in this category describes the overall delineation method.
  2. Vignettes with generic use cases: multiple delineations, delineation around a point of interest.

The distinction between these two categories is also indicated on the package website.

Note that:

  • Many of the code chunks are not evaluated as they contain functions that have not been written yet.
  • As I suggest in some vignettes, some preprocessing steps and API calls can be replaced by package data to make sure that the vignettes is focused on the core task.
  • It might be helpful to have a separate vignette on OSM functions, but I am not sure.
  • I expect more use cases in the second category, so feel free to suggest essential vignette topics that slipped my mind.

Vignettes ready for review:

Delineation:

  • 1. The method
  • 2. Corridor delineation
  • 3. Corridor segmentation
  • 4. Riverspace delineation

Use cases:

  • 5. Multiple delineations
  • 6. Study area around a POI

I hope this helps us sharpen our understanding of the package functionality and design.

@cforgaci cforgaci self-assigned this Aug 26, 2024
@cforgaci cforgaci added the documentation Improvements or additions to documentation label Aug 27, 2024
@cforgaci cforgaci linked an issue Aug 27, 2024 that may be closed by this pull request
@cforgaci cforgaci requested a review from fnattino August 28, 2024 09:36
@cforgaci cforgaci marked this pull request as ready for review August 28, 2024 09:36
Copy link
Collaborator

@meiertgrootes meiertgrootes left a 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.

cforgaci and others added 3 commits August 30, 2024 11:04
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
Co-authored-by: Meiert Willem Grootes <[email protected]>
Copy link
Contributor

@fnattino fnattino left a 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.

vignettes/multiple-cities.Rmd Outdated Show resolved Hide resolved
@cforgaci
Copy link
Contributor Author

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.

Thanks @meiertgrootes! I tried to address all the comments.

@cforgaci cforgaci mentioned this pull request Aug 31, 2024
2 tasks
@cforgaci
Copy link
Contributor Author

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.

Perfect, thanks for the review!

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.

I created #22 for this.

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.

This PR is ready to be merged.

@cforgaci cforgaci merged commit c2c187a into main Aug 31, 2024
7 of 8 checks passed
@cforgaci cforgaci deleted the 14-vignettes-cf branch August 31, 2024 08:08
@cforgaci cforgaci mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add vignettes
5 participants