PUBDEV-7366: GAM cross-validation #4869

wendycwong · 2020-08-22T00:55:43Z

This PR completes the work required in JIRA: https://0xdata.atlassian.net/browse/PUBDEV-7366

There are only 7 files to review. The other four are tests.

Users can now use cross-validation to find the best alpha/lambda values when building a GAM model. In the near future, we will be adding more hyper-parameters search capability. Stay tuned...

I have added to Java backend to ensure that cross-validation works for GAM. I added tests in R, Java and Python to make sure everything checks out.

h2o-algos/src/main/java/hex/gam/MatrixFrameUtils/GamUtils.java

h2o-algos/src/main/java/hex/gam/MatrixFrameUtils/GenerateGamMatrixOneColumn.java

h2o-genmodel/src/main/java/hex/genmodel/algos/gam/GamMojoModelBase.java

h2o-algos/src/test/java/hex/gam/GamCVTest.java

maurever

@wendycwong, thank you for this PR. It looks good to me. I have just a note about using CamelCase style as much as possible to keep our Java code clear and readable.

maurever · 2020-09-01T11:15:56Z

h2o-algos/src/main/java/hex/gam/GAM.java

@@ -31,7 +35,15 @@


 public class GAM extends ModelBuilder<GAMModel, GAMModel.GAMParameters, GAMModel.GAMModelOutput> {
-
+  double[][] _knots;
+  boolean _cv_on = false; // set to true if cross-validation is enabled


Do we have any other internal rule for using an underscore in Java code than at the beginning of a parameter? Because the standard is to use CamelCase. It is not clear to have two different styles in our Java code.

Suggested change

boolean _cv_on = false; // set to true if cross-validation is enabled

boolean _cvOn = false; // set to true if cross-validation is enabled

I think _cvOn is a good idea. I changed it.

h2o-algos/src/main/java/hex/gam/GAM.java

Pscheidl · 2020-09-08T10:17:52Z

I'll be finishing this PR as Wendy is on vacation.

PUBDEV-7366: Added processing to choose best lambda/alpha from cross-validation results PUBDEV-7366: Split init into two parts to accomodate cross-validation. PUBDEV-7366: Incorporate Pavel code review comments. incorporate veronika comments

Pscheidl · 2020-09-10T17:14:50Z

GamMojoModelTest fixed. One failure was resolved by rebasing (the NaN problem that's already fixed on master). I'll go through the code one more time tomorrow and then merge or ask for further reviews eventually.

Pscheidl · 2020-09-11T10:09:14Z

Added two more tests for multinomial and regression, did some minor code polishing (was not really required) and did one micro-optimization - validation keys were copied to the model for no reason. Just moved the pointer there 😉 .

Waiting for tests. Feel free to review @michalkurka and others. No big changes since the recent reviews.

…ing validation keys to model - small optimization.

michalkurka · 2020-09-11T15:52:11Z

h2o-algos/src/main/java/hex/gam/GAM.java

+        best_lambda = g._output._best_lambda;
+      }
+
+      g.write_lock(_job);


this looks like a copy&paste from GLM; it doesn't seem needed here because the GAM models are not modified here

michalkurka · 2020-09-11T15:55:45Z

h2o-algos/src/main/java/hex/gam/GAM.java

-      init(true);     //this can change the seed if it was set to -1
+      if (_doInit)                  // disable when in CV and building the main model
+        init(true);       //this can change the seed if it was set to -1
+      validateGamParameters();


I wonder why do we need this, however, if we keep _doInit then we should do

if (_doInit) {
init(true)
} else {
validateGamParameters();
}

however, skipping init is suspicious

Thank you for catching this. I though this is there because of the way GAM is instantiated, but it seems to be false theory. I'll look into it.

The only things that seems to be really effectively required from init is the validateGamParameters anyway.

h2o-algos/src/main/java/hex/gam/MatrixFrameUtils/GenerateGamMatrixOneColumn.java

michalkurka

Thank you @Pscheidl

Pscheidl · 2020-09-22T10:59:11Z

There are 4 other key management issues, solved 3 of them, working on the last one. Those were shadowed by the former problems.

Pscheidl · 2020-09-22T11:20:01Z

Resolved. One validation frame lifecycle was to blame - it was not properly deleted, as the IcedHashSet was not initialized if validation frame was declared by the user. It was only initialized when cross validation was done. This is no longer true and all the tests pass locally. An important issue the newly added tests caught.

One more round on Jenkins and we're good to go. Minor change really in terms of code.

Pscheidl · 2020-09-22T15:08:27Z

This one dataset is reused and it's deleted in remove_impl of GamModel. That's the original design. I just made sure it's deleted in all two cases (user-provided validation data) and not just xval.

Pscheidl · 2020-09-22T16:14:23Z

Oh, now I see what you're asking about. Let me fix that really quickly.

* PUBDEV-7366: Enabled and support GAM cross-validation on backend. PUBDEV-7366: Added processing to choose best lambda/alpha from cross-validation results PUBDEV-7366: Split init into two parts to accomodate cross-validation. PUBDEV-7366: Incorporate Pavel code review comments. incorporate veronika comments * JUnit tests for multinom/regression. Small code style fixes. Not copying validation keys to model - small optimization. * Always evaluate init on xval. Test key retention/removal. * Produce Frame with even chunk layout for spline calculations. * Assign validKeys and update job in one block. * Remove commented unused code * Proper cleanup of xval frames. * Remove unused Scope.exit(). Simplify model locking. Co-authored-by: Pavel Pscheidl <pavel@h2o.ai>

This reverts commit dcfc82f.

* PUBDEV-7366: Enabled and support GAM cross-validation on backend. PUBDEV-7366: Added processing to choose best lambda/alpha from cross-validation results PUBDEV-7366: Split init into two parts to accomodate cross-validation. PUBDEV-7366: Incorporate Pavel code review comments. incorporate veronika comments * JUnit tests for multinom/regression. Small code style fixes. Not copying validation keys to model - small optimization. * Always evaluate init on xval. Test key retention/removal. * Produce Frame with even chunk layout for spline calculations. * Assign validKeys and update job in one block. * Remove commented unused code * Proper cleanup of xval frames. * Remove unused Scope.exit(). Simplify model locking. Co-authored-by: Pavel Pscheidl <pavel@h2o.ai> (cherry picked from commit dcfc82f)

wendycwong requested review from Pscheidl, maurever, koniecsveta and michalkurka August 22, 2020 00:55

wendycwong force-pushed the PUBDEV_7366_gam_cross_validation branch 5 times, most recently from 3b622d8 to e1eed2e Compare August 25, 2020 18:13

Pscheidl reviewed Aug 26, 2020

View reviewed changes

wendycwong requested a review from Pscheidl August 27, 2020 00:33

wendycwong force-pushed the PUBDEV_7366_gam_cross_validation branch from e1eed2e to 930e5e0 Compare August 27, 2020 16:30

Pscheidl approved these changes Aug 27, 2020

View reviewed changes

wendycwong force-pushed the PUBDEV_7366_gam_cross_validation branch 4 times, most recently from 18a3b4f to 578911e Compare September 1, 2020 04:09

maurever approved these changes Sep 1, 2020

View reviewed changes

wendycwong force-pushed the PUBDEV_7366_gam_cross_validation branch 2 times, most recently from 3a4ebe9 to 45c41b3 Compare September 4, 2020 00:43

Pscheidl self-assigned this Sep 8, 2020

Pscheidl force-pushed the PUBDEV_7366_gam_cross_validation branch from 45c41b3 to 58fb008 Compare September 10, 2020 12:51

Pscheidl requested review from Pscheidl and removed request for Pscheidl September 10, 2020 12:52

JUnit tests for multinom/regression. Small code style fixes. Not copy…

50a0382

…ing validation keys to model - small optimization.

michalkurka reviewed Sep 11, 2020

View reviewed changes

Always evaluate init on xval. Test key retention/removal.

c40a93b

Pscheidl force-pushed the PUBDEV_7366_gam_cross_validation branch 10 times, most recently from 1e005ce to c40a93b Compare September 20, 2020 17:56

Pscheidl reviewed Sep 21, 2020

View reviewed changes

h2o-algos/src/main/java/hex/gam/MatrixFrameUtils/GenerateGamMatrixOneColumn.java Show resolved Hide resolved

Pavel Pscheidl added 3 commits September 21, 2020 11:29

Produce Frame with even chunk layout for spline calculations.

7b0c6ba

Assign validKeys and update job in one block.

6c87883

Remove commented unused code

67b6691

honzasterba approved these changes Sep 21, 2020

View reviewed changes

michalkurka approved these changes Sep 21, 2020

View reviewed changes

Proper cleanup of xval frames.

386e473

Remove unused Scope.exit(). Simplify model locking.

50f5038

Pscheidl merged commit dcfc82f into rel-zeno Sep 23, 2020

Pscheidl deleted the PUBDEV_7366_gam_cross_validation branch September 23, 2020 07:02

michalkurka pushed a commit that referenced this pull request Sep 28, 2020

Revert "PUBDEV-7366: GAM cross-validation (#4869)"

f580b0b

This reverts commit dcfc82f.

h2o-ops mentioned this pull request May 14, 2023

Make sure GAM works with cross-validation #8271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PUBDEV-7366: GAM cross-validation #4869

PUBDEV-7366: GAM cross-validation #4869

wendycwong commented Aug 22, 2020 •

edited

Loading

maurever left a comment

maurever Sep 1, 2020

wendycwong Sep 2, 2020

Pscheidl commented Sep 8, 2020

Pscheidl commented Sep 10, 2020

Pscheidl commented Sep 11, 2020

michalkurka Sep 11, 2020

michalkurka Sep 11, 2020

Pscheidl Sep 12, 2020

Pscheidl Sep 14, 2020

michalkurka left a comment

Pscheidl commented Sep 22, 2020

Pscheidl commented Sep 22, 2020 •

edited

Loading

Pscheidl commented Sep 22, 2020

Pscheidl commented Sep 22, 2020

	boolean _cv_on = false; // set to true if cross-validation is enabled
	boolean _cvOn = false; // set to true if cross-validation is enabled

PUBDEV-7366: GAM cross-validation #4869

PUBDEV-7366: GAM cross-validation #4869

Conversation

wendycwong commented Aug 22, 2020 • edited Loading

maurever left a comment

Choose a reason for hiding this comment

maurever Sep 1, 2020

Choose a reason for hiding this comment

wendycwong Sep 2, 2020

Choose a reason for hiding this comment

Pscheidl commented Sep 8, 2020

Pscheidl commented Sep 10, 2020

Pscheidl commented Sep 11, 2020

michalkurka Sep 11, 2020

Choose a reason for hiding this comment

michalkurka Sep 11, 2020

Choose a reason for hiding this comment

Pscheidl Sep 12, 2020

Choose a reason for hiding this comment

Pscheidl Sep 14, 2020

Choose a reason for hiding this comment

michalkurka left a comment

Choose a reason for hiding this comment

Pscheidl commented Sep 22, 2020

Pscheidl commented Sep 22, 2020 • edited Loading

Pscheidl commented Sep 22, 2020

Pscheidl commented Sep 22, 2020

wendycwong commented Aug 22, 2020 •

edited

Loading

Pscheidl commented Sep 22, 2020 •

edited

Loading