GH-16312 constrainted glm issues [nocheck]

[wendycwong]

This PR fixes problems in #16312

I fixed three issues here:

1. constrained GLM finished before constraint conditions are satisfied. There is a bug in gradient check and I fixed it. However, the constraint conditions are not satisfied at the end of the iteration. This is caused by either line search failure or no progress was made. However, if you compare the results with and without the constraints, the coefficients with constraints are closer to the constraints then the ones from model without constraints.
2. When specifying beta constraints with only the lower bounds, npe values occurred. I fixed this one by checking to make sure the beta constraint is not null before accessing its elements.
3. A set of three linear constraints are evaluated to be duplicated or conflicting when the constraints are neither. I fixed this error also because I dropped the last column/rows which are supposed to be for the constant. However, in generating the constraints matrix, I did not include the constant.

[maurever]

Thanks @wendycwong . I suggest one improvement; otherwise, LGTM.

[maurever]

You wrote: no need to check anything. But what about adding an assertion at the end that the model is not None? I suggest adding a try-catch and throwing a clear message instead of letting the test fail (in case of some problem).

[tomasfryda]

If the model is None then it would fail in model.train(...). What benefit would an assert have (especially if it would be at the end)? Or did you mean something like:

    model = glm(**params)
+   assert model is not None, "The model is None"
    model.train(x = predictors, y = response, training_frame = train_data)
    print(model.coef())
    print(glm.getConstraintsInfo(model))

?

If so, I don't understand the reason of adding the try except clause.

(Sorry if it is obvious, I haven't properly started the review yet so I might still get it.)

[maurever]

The advantage of failing with an assert message is to explain why it failed. If the NPE occurs and we know the potential problem, we can explain it in the message and save time in the future. We can also catch only NPE and other errors shown as usual.

I suggest improving the message from "The model is None" to something like "The model training failed with NPE. It could be caused by beta constraints setting."

[wendycwong]

Without my fix, the test will fail. However, after my fix, the NPE is no longer there. However, I did add a test because I know the beta constraints are always satisfied. Hence, I check that the beta constraints are always satisfied.

[wendycwong]

From @maurever , she is not comfortable for tests without some kind of assert statement. However, @tomasfryda feels that if the assert statement is too general, it is not very meaningful.

Taking both of your suggestions, I added assert statements that will check and make sure that the beta constraints are satisfied at the end of the model building process.

[maurever]

Hi @wendycwong. Thanks for improving the test. I went through the PR again and found nothing else to fix. I just saw that the Jenkins test has not been passed. But it may be due to infrastructure reasons. Please check that; otherwise, LGTM.