[jvm-packages] Deprecate constructors with implicit missing value. #7225
Conversation
|
Hey @Craigacp , could you please help take a look? |
|
Looks fine to me. You could modify the javadoc in I've only ever used the sparse DMatrix constructors though, so I'm not sure how changing the default will affect users. |
|
@Craigacp Thanks for the suggestion! @RAMitchell I'm worried that changing the |
Codecov Report
@@ Coverage Diff @@
## master #7225 +/- ##
=======================================
Coverage 82.63% 82.64%
=======================================
Files 13 13
Lines 4019 4021 +2
=======================================
+ Hits 3321 3323 +2
Misses 698 698
Continue to review full report at Codecov.
|
|
LGTM, |
| */ | ||
| @Deprecated | ||
| public DMatrix(float[] data, int nrow, int ncol) throws XGBoostError { | ||
| long[] out = new long[1]; | ||
| XGBoostJNI.checkCall(XGBoostJNI.XGDMatrixCreateFromMat(data, nrow, ncol, 0.0f, out)); |
There was a problem hiding this comment.
Could we change the default missing value to Float.Nan here instead of change everywhere
During debugging #7210 , I found the java implementation has a constructor the uses 0 as the default value for
missing, which doesn't make sense since 0 is a meaningful input for XGBoost.NaNwould be a better choice and is in fact used by both Python and R. However, I don't think it's a good idea for us to swap the missing value silently. Therefore, I propose that we deprecate those constructors and let users specifymissingexplicitly.