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

getResamplingIndices(): Translate inner resampling indices to outer indices #2413

Merged
merged 15 commits into from Oct 4, 2018

Conversation

2 participants
@pat-s
Copy link
Member

pat-s commented Aug 15, 2018

fixes #2409

So they match the task indices.

The test I added uses a basic "CV" with predefined indices (fixed = TRUE) since this resamp method groups the indices (1:30, 31:60, 61:90, etc) and the translation checking is more easy to understand.

The implementation is ofc hard to review since it uses three nested map() calls. But I think there is no other way to implement this.

The test essentially checks that in the inner inds have values that go up to the max index value of the task (in this case 150). Before, the max of inner inds was "obs - (obs/nfolds)", which evaluates to 120 in this test with 150 obs.

Can only be merged after #2412 since it uses code from this PR.

pat-s added some commits Aug 15, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Aug 16, 2018

As a side not: I moved purrr from suggest to imports as we use it in various functions now.
Its not about the simple map() function but about the enhanced derivates such as imap() or pmap() that make it worth using purrr.

The type safetyness of map_int() and others can be seen as additional syntactic sugar.

pat-s added some commits Aug 16, 2018

merge master
Merge branch 'master' into getresampinds

# Conflicts:
#	DESCRIPTION

@pat-s pat-s requested a review from larskotthoff Oct 1, 2018

pat-s and others added some commits Oct 1, 2018

@larskotthoff

This comment has been minimized.

Copy link
Contributor

larskotthoff commented Oct 2, 2018

There seem to be a lot of spurious/outdated changes in this branch. Could you please check that?

pat-s and others added some commits Oct 2, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Oct 4, 2018

have to check what's wrong in the example.

p = resample(tune_wrapper, ct, outer, show.info = FALSE,
extract = getTuneResult)

inner_inds = getResamplingIndicesNew(p, inner = TRUE)

This comment has been minimized.

@larskotthoff

larskotthoff Oct 4, 2018

Contributor

Typo here? Tests fail.

@larskotthoff

This comment has been minimized.

Copy link
Contributor

larskotthoff commented Oct 4, 2018

Thanks, merging.

@larskotthoff larskotthoff merged commit 1ad87b3 into master Oct 4, 2018

2 of 5 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@larskotthoff larskotthoff deleted the getresampinds branch Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment