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

PUBDEV-8024: Allow to use user given fold column where not all folds are represented #5345

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

michalkurka
Copy link
Contributor

No description provided.

@michalkurka michalkurka force-pushed the michalk_flexible-folds branch 3 times, most recently from fa80a9f to f2728d9 Compare February 24, 2021 18:29
Copy link
Contributor

@sebhrusen sebhrusen left a comment

Choose a reason for hiding this comment

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

Found this mapping/inverseMapping logic a bit confusing as it seems to make sense only in one use-case (user defined, non-sequential folds).
Couldn't we produce the correct mapping in one shot?

h2o-core/src/main/java/hex/FoldAssignment.java Outdated Show resolved Hide resolved
h2o-core/src/main/java/hex/FoldAssignment.java Outdated Show resolved Hide resolved
h2o-core/src/main/java/hex/FoldAssignment.java Outdated Show resolved Hide resolved
h2o-core/src/main/java/hex/FoldAssignment.java Outdated Show resolved Hide resolved
h2o-core/src/main/java/hex/FoldAssignment.java Outdated Show resolved Hide resolved
@michalkurka
Copy link
Contributor Author

@seb please take another look, I simplify it so that for "internal" fold there is no mapping created

I am also using TransformWrappedVec to avoid propagating the mapping to the outside world

Copy link
Contributor

@sebhrusen sebhrusen left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

h2o-core/src/main/java/water/fvec/TransformWrappedVec.java Outdated Show resolved Hide resolved
@michalkurka
Copy link
Contributor Author

@sebhrusen thank you for the review and for pushing me in the right direction, I think it is now cleaner and it doesn't leak the implementation details outside of FoldAssignment

@michalkurka michalkurka merged commit 012c610 into master Feb 26, 2021
@michalkurka michalkurka changed the title Allow to use user given fold column where not all folds are represented PUBDEV-8024: Allow to use user given fold column where not all folds are represented Mar 23, 2021
michalkurka pushed a commit that referenced this pull request Mar 23, 2021
We changed the way cv folds are assigned in #5345

This affects fold specification that doesn't start from 0, eg. 1, 2, 3

Old way was to use modulo operator resulting in effective fold assignment:
1, 2, 0

New way is to respect the fold order, resulting in fold assignment 0, 1, 2
flaviusburca pushed a commit to mware-solutions/h2o-3 that referenced this pull request Apr 21, 2021
We changed the way cv folds are assigned in h2oai#5345

This affects fold specification that doesn't start from 0, eg. 1, 2, 3

Old way was to use modulo operator resulting in effective fold assignment:
1, 2, 0

New way is to respect the fold order, resulting in fold assignment 0, 1, 2

(cherry picked from commit 8d5cc98)
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.

2 participants