Refactoring to accommodate new corridor delineation - Githubissues

CityRiverSpaces / CRiSp

CRiSp (City River Spaces) provides tools to automate the morphological delineation of riverside urban areas.

https://cityriverspaces.github.io/CRiSp/

Apache License 2.0

0 stars 0 forks source link

Refactoring to accommodate new corridor delineation #39

Closed fnattino closed 5 days ago

fnattino commented 2 weeks ago

Addressing #36

cforgaci commented 1 week ago

@fnattino, I drafted the osmdata module. Please have a look when you have time. I can review the other parts you worked on this afternoon.

fnattino commented 1 week ago

Hi @cforgaci, thanks! Sorry, I am running a bit late with this - I am working on it at the moment, will ping you as soon as I am done, but it will probably take me some more time this afternoon.

fnattino commented 1 week ago

Hi @cforgaci, I finally managed to get through the delineate, corridor and network modules. Still have to add tests, I will work on that tomorrow. In the next couple of days, feel free to review and provide feedback if you have time!

fnattino commented 1 week ago

@cforgaci I see we still have test data in test/testdata, but both the gpkg files as well as the scripts are now replaced by the packaged data - can I remove them?

cforgaci commented 1 week ago

@cforgaci I see we still have test data in test/testdata, but both the gpkg files as well as the scripts are now replaced by the packaged data - can I remove them?

Yes, please!

fnattino commented 1 week ago

Hi @cforgaci , I think this is getting close to done. I have left one suggestion above, then we should decide whether to update the vignette here or as part of a new PR - we can anyway discuss on Monday.

fnattino commented 1 week ago

@cforgaci The suggestion on the UTM zone utility function has been included, as well as fixes for the long URLs in the docstrings. I think that once the vignettes are in, we can merge this one?

cforgaci commented 1 week ago

@fnattino, I updated the core vignettes and all checks seem to be passing now. Feel free to merge after you review the latest updates.

fnattino commented 6 days ago

Hi @cforgaci, very nice! Just two comments/questions:

Should we maybe update the getting-osm-data.Rmd vignette to only illustrate the usage of the helper functions (get_osm_bb, get_osm_city_boundary, get_osm_river, etc. ) and of the all-in-one get_osmdata instead of showing all the internal working of the functions?
Before making the first release of the package, I am thinking that it would be best to separate documentation vignettes from design vignettes. We could either hide the content and leave the vignette there with a sign like "work in progress" or move the content that is not yet implemented to another directory (package-design?). This is to really make it clear what is there and what is not, and maybe what is coming..

The latter especially can be addressed in a separate PR..

cforgaci commented 5 days ago

@fnattino, I hope both issues are solved now.

Should we maybe update the getting-osm-data.Rmd vignette to only illustrate the usage of the helper functions (get_osm_bb, get_osm_city_boundary, get_osm_river, etc. ) and of the all-in-one get_osmdata instead of showing all the internal working of the functions?

Updated.

Before making the first release of the package, I am thinking that it would be best to separate documentation vignettes from design vignettes. We could either hide the content and leave the vignette there with a sign like "work in progress" or move the content that is not yet implemented to another directory (package-design?). This is to really make it clear what is there and what is not, and maybe what is coming..

I also moved the work-in-progress vignettes to a folder vignettes-drafts

fnattino commented 5 days ago

Thanks a lot @cforgaci for all the work on this, I leave you the honour of merging it :)