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

Add packaged DEM data #51

Merged
merged 12 commits into from
Dec 2, 2024
Merged

Add packaged DEM data #51

merged 12 commits into from
Dec 2, 2024

Conversation

fnattino
Copy link
Contributor

@fnattino fnattino commented Nov 29, 2024

(builds on top of #50, merge that first) Done

@fnattino fnattino linked an issue Nov 29, 2024 that may be closed by this pull request
@fnattino
Copy link
Contributor Author

GH actions are failing because the DEM data that I have added includes invalid pointers! It turns out SpatRaster objects cannot be easily serialized as rds/rda files:

"These classes hold a C++ pointer to the data and they cannot be directly saved to a ".Rds" file or
used in cluster computing. They cannot be recovered from a saved R session either. See wrap or
writeRaster to work around that limitation."
https://cran.r-project.org/web/packages/terra/terra.pdf

One can bypass this by using the dedicated saveRDS/readRDS functions. Can one make these work in the current packaged data setup? If not, we might as well store the DEM as a tiff file?

@cforgaci
Copy link
Contributor

cforgaci commented Dec 1, 2024

One can bypass this by using the dedicated saveRDS/readRDS functions. Can one make these work in the current packaged data setup? If not, we might as well store the DEM as a tiff file?

I did not know about this. If saveRDS/readRDS does not work in the current package structure, then the next option is to place a tiff into inst/extdata/. In this case, we need to indicate to the user that the dem needs to be loaded with system.file() separately as it is not made available automatically when the package is attached with library(CRiSp).

@fnattino fnattino marked this pull request as ready for review December 2, 2024 10:49
@fnattino
Copy link
Contributor Author

fnattino commented Dec 2, 2024

Using terra::saveRDS works, but one would then "manually" call terra::readRDS to read the serialized object. A workaround is what I have currently implemented, which wraps the DEM into a PackedSpatRaster object that can be normally serialized/deserialized with the standard R package infrastructure. The drawback: one needs to call "terra::unwrap()" on the Bucharest DEM object before using it - but this is clearly stated in the representation of PackedSpatRaster objects:

> CRiSp::bucharest_dem
[1] "This is a PackedSpatRaster object. Use 'terra::unwrap()' to unpack it"

Is this an acceptable solution?

@cforgaci
Copy link
Contributor

cforgaci commented Dec 2, 2024

Is this an acceptable solution?

Yes, I think this is a good solution. Thanks!

Comment on lines -68 to -69
walk(
st_write_list,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When updating the name of the dataset, I have noticed this function probably had a placeholder for the name of the dataset (st_write_list -> bucharest)? Also I made a suggestion to replace walk for walk2, which seems to simplify quite a bit the overall snippet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, much better!

Copy link
Contributor

@cforgaci cforgaci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prima!

Comment on lines -68 to -69
walk(
st_write_list,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, much better!

@fnattino fnattino merged commit 35fa222 into main Dec 2, 2024
8 checks passed
@fnattino fnattino deleted the 49-add-dem-data-fn branch December 2, 2024 21:39
@fnattino fnattino mentioned this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DEM to packaged data
2 participants