topodb vignette review

[kadyb]

DONE:

• [x] 3. "The downloaded database consists of many files in the Geography Markup Language format." <- use a shortcut "GML".
• [x] 5. "- fieldworks" <- I would leave a dot at the end.
• [x] 10. "The last type of analysis in this vignette is uses spatial buffers." <- "is uses"?
• [x] 9. "plot(bridges, add = TRUE, pch = 21, cex = 2)" <- Why cex = 2? Now the points look like buffers.
• [x] 7. data/PL.PZGiK.332.1801/BDOT10K/ <- lowercase "K".
• [x] 13. Remove files_name var (just print).
• [x] 15. "because this file contains zero features." <- "zero objects (features)"
• [x] 12. geodb_description.Rmd probable should be generalized to refer to two databases - Geographic and Topographic.
• [x] 14. Add sentence about "land cover".
• [x] 6. "The purpose of this vignette" <- In the rest of the articles we use the word "exercise". It needs to be unified.
• [x] 17. Orthophotomap: Add a date of image.
• [x] 11. The names in the legend in "Bieszczadzki county" are truncated (https://github.com/r-spatial/sf/issues/1521).
• [x] 1. Italic in filenames doesn't work (*PL.PZGiK.332.1801__OT_SKDR_L.xml*). Maybe replace this with quotation marks. Note, this same problem is in orthophotomap vignette.
• [x] 16. "For each object (layer), we calculate its area separately in a loop." <- We use lapply now. Plus add some description of this.

[kadyb]

JN: I do not understand what do you mean here JN: I understand the code, though.. Where did you get this names? Are they universal? This should be explained

The files contain abbreviated names (PTRK - Pokrycie Terenu Roślinność Krzewiasta, PTWP - Pokrycie Terenu Woda Powierzchniowa, etc.). I just translated and simplified them ("shrublands", "water").

JN: I would suggest rewriting the code in this section. I have almost never seen the use of assign in the exploratory code

How do you think it should look like?

JN: what is "area land cover"??

Do you mean to add one sentence of description? "Land cover maps represent spatial information on different types (classes) of physical coverage of the Earth's surface, e.g. forests, grasslands, croplands, lakes, wetlands." (https://land.copernicus.eu/global/products/lc) "Land cover is the physical material at the surface of the earth. Land covers include grass, asphalt, trees, bare ground, water, etc." (https://en.wikipedia.org/wiki/Land_cover)

[Nowosad]

"The thematic scope includes, among others, information on: water network, communication network, land cover, buildings, technical structures, land use, protected areas, territorial division units." <- We should leave "among others" as we don't list all the available datasets, but only a sample part.

"includes" means that we only show some examples. "among others" is not necessary when you use "includes".

[Nowosad]

6. "The purpose of this vignette" <- In the rest of the articles we use the word "exercise". It needs to be unified.

I cannot find the term "exercise" anymore in the file.

[kadyb]

6. "The purpose of this vignette" <- In the rest of the articles we use the word "exercise". It needs to be unified. I cannot find the term "exercise" anymore in the file.

I mean DEM and Orthophotomap vignettes: "The purpose of this exercise is (...)".

[kadyb]

"The thematic scope includes, among others, information on: water network, communication network, land cover, buildings, technical structures, land use, protected areas, territorial division units." <- We should leave "among others" as we don't list all the available datasets, but only a sample part.

"includes" means that we only show some examples. "among others" is not necessary when you use "includes".

Maybe we should list all the thematic layers or list and write "and more" at the end. I think, this limited thematic scope list can be confusing for users not previously using this database.

Edit: I added all (9) category names in Topographic Database.

[kadyb]

@Nowosad could you review this vignette one more time?

[Nowosad]

Will do - sometime in the next day or two.

[Nowosad]

It looks good now. I just added some tiny modifications - https://github.com/kadyb/rgugik/commit/e6e2faee48fdf816f5d588a9bc43767762375818.

[kadyb]

Thanks!

After remove this line:

area_landcover = units::set_units(area_landcover, "m^2")

we have incorrect units.

[Nowosad]

Sorry for that - I (wrongly) expected different behavior from set_units(). It should be fixed now.

[kadyb]

No problem!