coin-or / Ipopt

COIN-OR Interior Point Optimizer IPOPT
https://coin-or.github.io/Ipopt
Other
1.41k stars 281 forks source link

CSR format returns upper, not lower, triangle #638

Open dpo opened 1 year ago

dpo commented 1 year ago

This is in relation to https://github.com/coin-or/Ipopt/discussions/618.

My linear solver requires the augmented system in CSR format. In the class that inherits from SparseSymLinearSolverInterface, I declared that I require the CSR_Format_0_Offset format. However, IPOPT returned the upper triangle of the augmented system, not the lower triangle as documented.

Is this an error in the documentation? It could be fixed by changing "CSR" to "CSC" everywhere in the docs :-), but it doesn't seem that that's what the authors intended.

Thanks!

svigerske commented 1 year ago

I think it's an error in the documentation. At least, that would be easier to fix. :)

The description of the class that does the conversion says

Class for converting symmetric matrices given in triplet format to matrices in compressed sparse row (CSR) format of the upper triangular part (or, equivalently, compressed sparse column (CSC) format for the lower triangular part).

Further, looking at some other linear solvers that request a CSR format, it seems that they work with the current setup. For example, the MA86 documentation says that it requires the following form:

The user must supply the lower triangular part of the matrix A in standard HSL format. This is a compressed sparse column format with the entries within each column ordered by increasing row index.

For MA97 it doesn't seem to matter whether it the upper or lower triangular is provided.

I would then rather not change the conversion routine, but only the docu of EMatrixFormat.

One could consider adding the possibility to request the lower triangular instead. (Pull requests are welcomed :))

dpo commented 1 year ago

I find it quite confusing that you would get the lower triangle if you requested the triplet format, but the upper triangle if you requested CSR. If you prefer only changing the documentation, I would suggest replacing CSR with CSC everywhere.

Alternatively, you could easily have the triplet format return the upper triangle (by swapping irow and jcol).

svigerske commented 1 year ago

Yes, it may be confusing, and it may be easy to change, but it would change the API or existing behavior. I can keep it in mind for the next major release. Renaming the enum entries from CSR to CSC would be easy then. If I were to change the triplet format to the upper triangular, it may no longer be suitable for MA27. So one would need to add a choice somewhere whether to get upper or lower triangles.

I don't think that there are plans for a major release anytime soon, so I like the harmless way of pointing out more clearly in the documentation what is returned - and that it is confusing.

dpo commented 1 year ago

Sounds fair, thank you!