njtierney / geotargets

Targets extensions for geospatial data
https://njtierney.github.io/geotargets/
Other
49 stars 4 forks source link

Add `tar_stars()` #33

Closed brownag closed 1 month ago

brownag commented 3 months ago

Adds format methods tar_stars(), tar_stars_proxy() for #14

brownag commented 3 months ago

It might be good to clarify that this currently works for stars raster cubes, but might not work for vector cubes or stars.proxy objects.

Good point about vector cubes, we should work up an example for that in a new issue. We can hash it out before merging this PR if need be.

In https://github.com/njtierney/geotargets/pull/33/commits/0e20c172f574efed855625ec06219c40eade9917 I added support for stars_proxy objects. tar_stars() now takes a proxy argument (default: FALSE) and there is a tar_stars_proxy() function for shorthand. I think tar_stars_proxy() could call tar_stars() but would need to think more about the deparsing/evaluation of the arguments to make it work.

We might want to explore these other formats a bit and depending on the findings of that exploration, perhaps call this tar_stars_raster even though both raster cubes and vector cubes have the same class (stars).

I think we should open an issue for this and look into some vector data cube examples--I am not a heavy user of stars, and can't say I have a ton of experience with that specific application--though I want to investigate now.

As I understand it creating vector data cubes requires reading the vector data with sf::st_read()/sf::read_sf() and then converting to stars with stars::st_as_stars(). So, we definitely might want to provide a target format that eases this process, but right now if the user has a stars or stars_proxy object already made, I think what is implemented in this PR "works"--open to a counterexample!

Aariq commented 2 months ago

If you'd like, I can attempt to bring this up to speed with the master branch. That'll make it easier to review and discuss.

brownag commented 2 months ago

If you'd like, I can attempt to bring this up to speed with the master branch. That'll make it easier to review and discuss.

Thanks for the offer, I've been trying to keep up with changes... and wanted to make sure I went over everything so got the new master branch merged in here.

I am going to re-request review on this now. I think all prior comments/changes have been resolved and/or referred to in an open issue.

njtierney commented 2 months ago

Fantastic, thanks @brownag ! :)

brownag commented 1 month ago

Looking good. Only other thing is can you update NEWS.md now that we have it?

Thanks, done!