geocompx / geocompy

Geocomputation with Python: an open source book and online resource for getting started in this space
https://py.geocompx.org/
Other
260 stars 47 forks source link

JN: review chapter 1 #194

Closed Nowosad closed 9 months ago

Nowosad commented 11 months ago

I read the first chapter, updated its style, fixed some typos, and rewrote some paragraphs (nothing controversial, I believe).

I also have some other comments:

  1. [x] I added a few comments to the .qmd file starting with <!--jn: comment-->; there are also some unresolved TODO comments in the text.
  2. [x] Can we cite packages mentioned in the book somehow? It is especially important for the main ones, such as geopandas, etc.
  3. [x] What is the preferred style: "the .plot method" or "the .plot() method"?
  4. [x] There are a few very wide warnings in this chapter, e.g., "/tmp/ipykernel_513521/2017122361.py:1: UserWarning: Geometry is in a geographic CRS. Results from 'centroid' are likely incorrect. Use 'GeoSeries.to_crs()' to re-project geometries to a projected CRS before this operation." What to do with them? Suppress or wrap and explain?
  5. [x] The code style of the 'MultiPolygon'` geometry creation in 1.2.5 is different from the rest..
  6. [x] I think we should avoid using GCRS for calculating areas (even as an example -- I think it sends a wrong message)... /tmp/ipykernel_534270/138307179.py:1: UserWarning: Geometry is in a geographic CRS. Results from 'area' are likely incorrect. Use 'GeoSeries.to_crs()' to re-project geometries to a projected CRS before this operation.
  7. [x] There is an introduction to what vector objects are, but a similar section for raster data is missing (something like https://r.geocompx.org/spatial-class#raster-data)
  8. [x] In general, I think the explanation figures are excellent. I only have a comment to https://py.geocompx.org/01-spatial-data#fig-rasterio-structure -- I think the font of the example boxes it too small, can you enlarge it?
  9. [x] We could use Blocks (https://quarto.org/docs/authoring/markdown-basics.html#callout-blocks) instead of normal text for some side-explanations.
michaeldorman commented 11 months ago

Thanks @Nowosad ! Will go over these comments

michaeldorman commented 11 months ago

The code style of the 'MultiPolygon'` geometry creation in 1.2.5 is different from the rest..

Good point! I now tried to improve and clarify this part (https://github.com/geocompx/geocompy/commit/7a12a166ce000f9a3b9726817dc97a3accb481e2)

michaeldorman commented 11 months ago

I think we should avoid using GCRS for calculating areas (even as an example -- I think it sends a wrong message)... /tmp/ipykernel_534270/138307179.py:1: UserWarning: Geometry is in a geographic CRS. Results from 'area' are likely incorrect. Use 'GeoSeries.to_crs()' to re-project geometries to a projected CRS before this operation.

Agree, now modified this part to avoid this kind of example (https://github.com/geocompx/geocompy/commit/b56fbb547e48f24dd512d3754aa7d2a94f99c6b9)

michaeldorman commented 11 months ago

What is the preferred style: "the .plot method" or "the .plot() method"?

I've used .plot, to be in agreement with how functions are specified (e.g. gpd.GeoSeries, not gpd.GeoSeries())

michaeldorman commented 11 months ago

There are a few very wide warnings in this chapter, e.g., "/tmp/ipykernel_513521/2017122361.py:1: UserWarning: Geometry is in a geographic CRS. Results from 'centroid' are likely incorrect. Use 'GeoSeries.to_crs()' to re-project geometries to a projected CRS before this operation." What to do with them? Suppress or wrap and explain?

I suggest https://github.com/geocompx/geocompy/pull/196 for this, will be happy to hear your thoughts

michaeldorman commented 11 months ago

In general, I think the explanation figures are excellent. I only have a comment to https://py.geocompx.org/01-spatial-data#fig-rasterio-structure -- I think the font of the example boxes it too small, can you enlarge it?

Thanks! I made the text larger in https://github.com/geocompx/geocompy/commit/16408f4497c0eaf292b5a691aff45e64dbbcb6b4

michaeldorman commented 11 months ago

I added a few comments to the .qmd file starting with ; there are also some unresolved TODO comments in the text.

Thanks, the comments are addressed here (https://github.com/geocompx/geocompy/commit/1079d0d4c021ca8d8a203afc0e494d329e61b786) (while doing it I somehow reverted the .envelope change following @Robinlovelace comments, now brought them back and hoping that it didn't mess up anything else, sorry...)

michaeldorman commented 11 months ago

There is an introduction to what vector objects are, but a similar section for raster data is missing (something like https://r.geocompx.org/spatial-class#raster-data)

Added the intro from geocompr (https://github.com/geocompx/geocompy/commit/d91bfc7fa088eeccb352d881c56a5442be18baa7)

michaeldorman commented 10 months ago

We could use Blocks (https://quarto.org/docs/authoring/markdown-basics.html#callout-blocks) instead of normal text for some side-explanations.

We are already using it, so marking as complete (example below, unless you had something else in mind?)

Screenshot from 2023-11-06 17-04-03

Nowosad commented 10 months ago

Hi @michaeldorman -- yes, I had such blocks in mind (I am sure, you also have seen direct comments about adding new blocks in the rest of the chapters). Thanks!

Robinlovelace commented 10 months ago

Can we cite packages mentioned in the book somehow? It is especially important for the main ones, such as geopandas, etc.

@martinfleis how best to cite it?

michaeldorman commented 10 months ago

Can we cite packages mentioned in the book somehow? It is especially important for the main ones, such as geopandas, etc.

@martinfleis how best to cite it?

Based on Google the recommendation was to use the suggested citation from each package docs. I've just added the one for geopandas to test: https://py.geocompx.org/01-spatial-data#ref-kelsey_jordahl_2020_3946761

martinfleis commented 10 months ago

Zenodo is correct but better to cite the latest version, not 0.8.1. We are planning a paper that would work as a canonical citation but that is not even in a draft version yet.

michaeldorman commented 9 months ago

Can we cite packages mentioned in the book somehow? It is especially important for the main ones, such as geopandas, etc.

https://github.com/geocompx/geocompy/commit/620780a91116fbd132d2c1ca71c5d81c8aeee45f

Nowosad commented 9 months ago

Great @michaeldorman -- I've done something similar to geocompr recently.