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 tar_stars() #33

Merged
merged 28 commits into from
May 29, 2024
Merged

Add tar_stars() #33

merged 28 commits into from
May 29, 2024

Conversation

brownag
Copy link
Contributor

@brownag brownag commented Mar 15, 2024

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

  • Adds {sf}, {stars} and {ncmeta} to suggests

  • When mdim=TRUE tar_stars() and tar_stars_proxy() can use the GDAL Multidimensional Raster Data Model as a backend via stars::read/write_mdim()instead of stars::read/write_stars()

  • When ncdf=TRUE "read" methods can be toggled to use stars::read_ncdf() (instead of stars::read_stars() or stars::read_mdim()) which requires {ncmeta}

  • To be less repetitive I thinktar_stars_proxy() could call tar_stars(proxy=TRUE) but I still need to sort out how arguments would need to be deparsed to do this.

Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

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

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. 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).

R/tar-stars.R Outdated Show resolved Hide resolved
R/tar-stars.R Outdated Show resolved Hide resolved
R/tar-stars.R Outdated Show resolved Hide resolved
tests/testthat/test-tar-stars.R Outdated Show resolved Hide resolved
R/tar-stars.R Outdated Show resolved Hide resolved
R/tar-stars.R Outdated Show resolved Hide resolved
Copy link
Owner

@njtierney njtierney left a comment

Choose a reason for hiding this comment

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

Thanks so much for this, @brownag !

All looks great to me, here are some suggestions on pkg documentation and using rlang::check_installed()

@brownag
Copy link
Contributor Author

brownag commented Mar 16, 2024

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 0e20c17 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!

R/tar-stars.R Show resolved Hide resolved


 - substitute in options, read/write functions following njtierney#43
 - add support for reading netCDF via `stars::read_ncdf()` using njtierney#43
 - consolidate option getter usage for njtierney#34
@Aariq
Copy link
Collaborator

Aariq commented Apr 26, 2024

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
Copy link
Contributor Author

brownag commented Apr 27, 2024

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.

@brownag brownag requested review from Aariq and njtierney April 27, 2024 18:41
Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

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

Could you re-run devtools::document() to update tar_stars.Rd

R/tar-stars.R Outdated Show resolved Hide resolved
R/tar-stars.R Outdated Show resolved Hide resolved
brownag and others added 2 commits April 29, 2024 21:23
R/geotargets-package.R Outdated Show resolved Hide resolved
Copy link
Owner

@njtierney njtierney left a comment

Choose a reason for hiding this comment

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

Pending the changes from @Aariq and a minor change that I have about using @importFrom and a request to add an example of stars into the README, I think this is looking good :)

tests/testthat/test-tar-stars.R Show resolved Hide resolved
@njtierney
Copy link
Owner

Fantastic, thanks @brownag ! :)

@Aariq Aariq self-requested a review May 3, 2024 18:03
Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

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

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

R/tar-stars.R Outdated Show resolved Hide resolved
@brownag
Copy link
Contributor Author

brownag commented May 11, 2024

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

Thanks, done!

Aariq added 2 commits May 29, 2024 10:11
Merge commit '1816bcdd41bcdc6e7b770017db1cac9ecd074f46'

#Conflicts:
#	DESCRIPTION
#	R/geotargets-option.R
#	R/tar-terra-rast.R
#	README.Rmd
#	README.md
@Aariq Aariq merged commit 2753efe into njtierney:master May 29, 2024
7 checks passed
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.

3 participants