Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSR format returns upper, not lower, triangle #638

Open
dpo opened this issue Feb 1, 2023 · 4 comments
Open

CSR format returns upper, not lower, triangle #638

dpo opened this issue Feb 1, 2023 · 4 comments

Comments

@dpo
Copy link
Member

dpo commented Feb 1, 2023

This is in relation to #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
Copy link
Member

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 :))

svigerske added a commit that referenced this issue Feb 2, 2023
- CSR_Format gives the upper triangular matrix, not the lower
- see #638
@dpo
Copy link
Member Author

dpo commented Feb 6, 2023

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 added a commit that referenced this issue Feb 7, 2023
- triplet gives lower diagonal, but CSR gives upper diagonal
- see also #638
@svigerske
Copy link
Member

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
Copy link
Member Author

dpo commented Feb 7, 2023

Sounds fair, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants