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

Inconsistent warnings about converting to matrix #2201

Open
artpelling opened this issue Oct 9, 2023 · 1 comment
Open

Inconsistent warnings about converting to matrix #2201

artpelling opened this issue Oct 9, 2023 · 1 comment

Comments

@artpelling
Copy link
Member

At the moment there are inconsistencies about how to handle and warn about large Operators being converted to matrices in some algorithms. For example LTIModel._poles warns you about this in certain cases, LTIModel.to_matrices does not.

I generally like the idea that there is a default for the size of matrix up until the conversion will be done. In pymor.models.iosys there is sparse_min_size that achieves something similar. I think it would be helpful to do this on a global level and bake this into to_matrix. Then all algorithms could be written more concisely without handling these cases and the behaviour of matrix conversion can be tuned to the computing platform in a central place.

@pmli
Copy link
Member

pmli commented Oct 10, 2023

I would also be for having a more systematic way of dealing with this. I think it started with poles and hinf_norm (which moved to _poles and _linf_norm), and then was also added in MTReductor. I believe LTIModel.to_matrices is a bit different, because sparse matrices are not necessarily converted to dense matrices.

Another thing is that only warnings are raised. That makes it easy to shoot yourself in the foot, e.g., calling poles for a system of order 10,000 will call scipy.linalg.eig which is likely impossible to stop with Ctrl+C and you would need to somehow kill the Python process or wait for a very long time. (I think at some point the poles method had a force parameter, which would by default be False and raise an error for big orders, but then we removed it for some reason.)

For the first part, it sounds like a good idea to me to have a default option in to_matrix to specify when to warn about conversion from sparse to dense.

For the second part, one approach would be to raise errors instead of warnings. The user would then need to increase the size to avoid errors. That would make it annoying because of extra code, but would help avoid bad things from happening.

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

No branches or pull requests

2 participants