geocompx / geocompr

Geocomputation with R: an open source book
https://r.geocompx.org/
Other
1.58k stars 585 forks source link

Reviews - edition 2, round 1, reviewer 1 #785

Closed Nowosad closed 2 years ago

Nowosad commented 2 years ago

We now mention the origin of the downloads data.

We have updated this section.

E3 has been updated to make it more relevant. E4 has been removed.

We have updated the text to explain what the remotes package does. We have also mentioned the dependencies = TRUE argument which will download additional dependencies needed to reproduce subsequent chapters.

This has been fixed: https://github.com/Robinlovelace/geocompr/commit/dedbc5c31b93a76ea5996121e340ff5f335830de

The note is now:

The `[[` and `$` operators can also be used to select layers, for example with commands `multi_rast$landsat_1` and `multi_rast[["landsat_1"]]`.

We now include an alternative-to-pipe example:

world9_filtered = filter(world, continent == "Asia")
world9_selected = dplyr::select(world9_filtered, continent)
world9 = slice(world9_selected, 1:5)

Furthermore we provide more context-specific guidance: https://github.com/Robinlovelace/geocompr/commit/63908348ded498fa8acac3721233f3f0eef0bf58

Good point. We now also mention that tidyr is part of the tidyverse early in the chapter and that readers may want to install these packages. See https://github.com/Robinlovelace/geocompr/commit/f45a07c72301d44e13e670ba0a35f39ff0f86267 for the full set of changes in relation to this much-appreciated comment.

Fixed in https://github.com/Robinlovelace/geocompr/commit/ad199d9b079a45a0d6f497513b84ec716c15cc11

This is now done in https://github.com/Robinlovelace/geocompr/commit/be0eba1126fa3a2c143fc4b3969614e8e8d0436c and the previous commit.

The exercise has been updated to make it clear what we're referring to here. See https://github.com/Robinlovelace/geocompr/commit/7d6f8376c2d6e015cd806762ffdb3ecde4bcdf68 for details.

We now describe vect(). See https://github.com/Robinlovelace/geocompr/commit/250daff61a93b66599bc3f91be51ece383572f82. Also see #780 for discussion of a new section on terra's vector class system (we're not sure we will write about it but like the idea).

Good point. Fixed and improved in https://github.com/Robinlovelace/geocompr/commit/f2b7b41a95762cd905928b9cc01c36a1b43053be

Agreed, the updated prose is much clearer thanks to this comment: https://github.com/Robinlovelace/geocompr/commit/f1d079762f20de9309c12745b1764c91aac8c7b9

We have explored this and cannot find any instances where lwgeom is needed. We have therefore removed use of lwgeom and added a comment about lwgeom earlier in the chapter: https://github.com/Robinlovelace/geocompr/commit/32e89551c34e92058124a463dd6ee1e273021cf4

Robinlovelace commented 2 years ago

Impressive list of useful suggestions and good progress on tackling them. Next lot of reviewer comments incoming, sorry I took a long time to get on the case, busy days at work!

Robinlovelace commented 2 years ago

Heads-up guys I'm working on this now, need to get the review responses submitted!

Nowosad commented 2 years ago

Hi @Robinlovelace -- I plan to work on some of the issues during the weekend.

Robinlovelace commented 2 years ago

Great stuff Jakub, let's get the responses sent asap and then move on to the (hopefully easier but still tricky) Part II.

Robinlovelace commented 2 years ago

Heads-up @Nowosad and @jannes-m all the issues that I think were well suited to me (and one of Jakub's I did) are now fixed. I'm going to relax! Please take a look at this, I think we will be able to close this mega issue soon :crossed_fingers:

Nowosad commented 2 years ago

" [RL,JN] p.164 " st_transform_proj() from the lwgeom package allows for coordinate transformations to a wide range of CRSs, including the Winkel tripel projection:" It could be useful to describe when using lwgeom::st_transform_proj() instead of st_transform() is needed." -- I looked for any document/website showing the differences between GDAL->PROJ (st_transform()) and directly PROJ (st_transform_proj()), however I was unable to find anything...

Robinlovelace commented 2 years ago

Maybe we should add some test code with echo=FALSE, eval=FALSE to check if they're the same. I sense you know this area better than me Jakub.

Robinlovelace commented 2 years ago

Maybe we should add some test code with echo=FALSE, eval=FALSE to check if they're the same. I sense you know this area better than me Jakub.

Heads-up @Nowosad I'm looking at this now.