Skip to content

Conversation

@hfrick
Copy link
Member

@hfrick hfrick commented Nov 3, 2025

Closes #166

Some recent-ish changes in rsample are affecting spatialsample: rsample now prohibits LOO and has new type checkers.

The related breakages show up in the revdep tests for testthat, which is supposed to go out in the next week or so.

This PR removes test failures so that we can submit to CRAN (either after we get the 2-week deadline or, ideally, before testthat goes out).

Do you have bandwidth to look at the changes in the snapshots? I think some of the tests use LOO very intentionally to trigger helpful warnings from spatialsample - which might now be dead code. Edit: I've removed such a piece of dead code, the error when trying to do repeated LOO in spatial_vfold_cv(). And I've now seen the buffering.Rmd article error because it tries to use LOO for "leave-one-disc-out cross-validation". Taking this as confirmation that there was intent behind some of the LOO usage and pausing here so you can chip in before I do more :D

@hfrick hfrick requested a review from mikemahoney218 November 3, 2025 13:46
@mikemahoney218
Copy link
Member

I'll take a look either tonight or tomorrow!

@mikemahoney218
Copy link
Member

Looking into this, I suspect this is going to be a tricky update for spatialsample. As you noticed, we use vfold_cv to enable a few different types of CV, including leave one disc out which has a long history in spatial modeling. Importantly, even though this process starts by creating 1-row test sets, we then expand the test set using the radius argument -- so I think the object shouldn't be subject to any of the same restrictions as LOO CV rsets/rsplits are.

We could potentially check to see if v == nrow(data), use vfold or loo depending on the check result, and then "undo" everything loo_cv does if needed? I don't see a non-hacky way to keep this functionality otherwise. Would rsample be willing to secretly pass prevent_loo in dots to vfold_splits?

@mikemahoney218
Copy link
Member

Does this close #168 by the way? (Thank you!)

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.

Update snapshots

3 participants