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

Fix bug due to change in terra's SpatExtent #79

Merged
merged 1 commit into from Sep 10, 2023

Conversation

adamlilith
Copy link

SpatExtent objects no longer contain the @ptr slot. Using as.vector() around a SpatExtent object retrieves the extent coordinates.

SpatExtent objects no longer contain the @ptr slot. Using as.vector() around a SpatExtent object retrieves the extent coordinates.
Copy link
Owner

@rsbivand rsbivand left a comment

Choose a reason for hiding this comment

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

Thanks. However, this needs to condition on terra versions. Could you please check which version brought in the change? If say 3 months ago, we could also add that version as a minimum in DESCRIPTION, if more recent, we need to condition. We may need to condition in code anyway for users of Windows or macOS terra binaries from CRAN for R 4.2.

@rsbivand
Copy link
Owner

rsbivand commented Sep 9, 2023

The slot formerly ptr was renamed to pnt in rspatial/terra@c2b6697#diff-94c09e9a0dce65c379c47bd63c4c3fb115f0f3b516d145d1fda96458b979eebc (this is as in released 1.7-46), and to cpp in rspatial/terra@c5b34dd#diff-94c09e9a0dce65c379c47bd63c4c3fb115f0f3b516d145d1fda96458b979eebc. I'm a bit unsure how to proceed, since CMD check doesn't fail in nc_basic_spm_grass7. We'd need first to add a test that fails with 1.7-46, and then merge your PR, with further checks with 1.7-46, pre-1.7-46, and terra-master. @adamlilith what was your use case when you hit the problem? Best using nc_basic_spm_grass7.

@rhijmans
Copy link

rhijmans commented Sep 10, 2023

I do not think you need to condition on version. as.vector<SpatExtent> has been part of terra from the first CRAN release.

The breaking code uses the x@ptr$..... idiom. I do not consider that a part of the user interface because I would not be able to make changes to it (e.g., adding an argument) without breaking existing code in other packages. This is why I will regularly change its name. Currently it is x@pnt$... on CRAN and x@cpp$... on github. I warned package maintainers beforehand but this one in rgrass did not come up in the reverse dependency checking.

@rsbivand
Copy link
Owner

@rhijmans Thanks, I understand. No revdep checks are run on examples when GRASS is absent - all the examples run with GRASS. I'll adapt and apply.

@rsbivand rsbivand merged commit f2f8c51 into rsbivand:main Sep 10, 2023
@rhijmans
Copy link

Thanks. I should also have searched https://github.com/cran. Doing so today helped me find a few more cases that do not show up in reverse dependency checking.

@rsbivand
Copy link
Owner

I should have looked more carefully when writing the code originally. Revised version on CRAN now.

@adamlilith adamlilith deleted the adam_smith branch September 11, 2023 04:51
@adamlilith
Copy link
Author

Thanks @rsbivand -- and thanks for the details on terra, @rhijmans, as I didn't know that about the pointer changing name.

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.

None yet

3 participants