jfowkes / pycutest

Python interface to CUTEst
https://jfowkes.github.io/pycutest/
GNU General Public License v3.0
28 stars 11 forks source link

Fix invalid sparse Hessian dimensions while dropping fixed variables #62

Closed chrhansk closed 1 year ago

chrhansk commented 1 year ago

Describe the change(s)

The internal sparse Hessian function assumes wrong dimensions when creating the Hessian matrix. A difference occurs when n != n_full, i.e., if variables are dropped (e.g. instance BOX2). This results in a ValueError('row index exceeds matrix dimesions') being thrown by scipy.

Have you updated the documentation?

Yes

Have you included relevant unit tests?

No, I cannot get the test framework to work. But loading BOX2 and executing sphess should be sufficient.

jfowkes commented 1 year ago

Many thanks for the proposed fix @chrhansk!

@lindonroberts would you be able to look into this? I'm not that familiar with the n_free/n_fixed variable handling, and I suspect a similar issue may be present in the other sparse matrix functions (e.g. gradsphess etc).

chrhansk commented 1 year ago

That is a valid point, maybe we should add the BOX2 example and call pretty much all functions (Jacobian, Hessian, and the combinations) to ensure everything works correctly.

jfowkes commented 1 year ago

That is a valid point, maybe we should add the BOX2 example and call pretty much all functions (Jacobian, Hessian, and the combinations) to ensure everything works correctly.

Yes indeed, might be good to add BOX2 to the sparse problem functions unit test here: pycutest/tests/test_sparse_functionality.py

lindonroberts commented 1 year ago

Thanks @chrhansk - yes, I think all the underscore sparse functions (scons, slagjac, ...) would have the same issue. And I agree with @jfowkes that it would worthwhile adding BOX2 to the sparse unit tests in pycutest/tests/test_sparse_functionality.

jfowkes commented 1 year ago

@lindonroberts many thanks, I've made the suggested changes and all seems to work, please do review when you have some time.

lindonroberts commented 1 year ago

Looks good to me, thanks @jfowkes!