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

revert to condense from enforce, passing symmetric solver #613

Merged
merged 3 commits into from
Apr 8, 2021
Merged

revert to condense from enforce, passing symmetric solver #613

merged 3 commits into from
Apr 8, 2021

Conversation

gdmcbain
Copy link
Contributor

@gdmcbain gdmcbain commented Apr 8, 2021

This takes ex21 back to where it was before #591 and #596, thereby avoiding having to excise the spurious imaginary part introduced by the asymmetric eigensolver as in #611 or #612.

Eventually it might be possible to enforce rather than condense the Dirichlet conditions, but I don't think it's quite clear yet how to do this for generalized eigenvalue problems.

@kinnala
Copy link
Owner

kinnala commented Apr 8, 2021

I did not observe any spurious imaginary part, just a different dtype with zero imaginary part, but I think you are correct here in the sense that this is the proper way to solve a symmetric problem.

I changed the behavior of solve because I think it should deal with the most general case by default or otherwise the result can be surprising: wrong results without any warning.

@gdmcbain
Copy link
Contributor Author

gdmcbain commented Apr 8, 2021

I did not observe any spurious imaginary part, just a different dtype with zero imaginary part,

Ah, right, I didn't check that the spurious imaginary part was actually zero.

but I think you are correct here in the sense that this is the proper way to solve a symmetric problem.

Yeah, there's still all the other problems of #75 and I haven't totally decided that enforce can't work for something like this if used correctly; we'll see whether its original proponent chimes in. It will introduce zero eigenvalues for the Dirichlet degrees of freedom but that doesn't necessarily matter. I hadn't at first realized that #596 was only replacing the rows and not the columns. I think in elementary presentations (reference?) the column is also replaced so that symmetry is preserved but of course that's too inefficient for a csr_matrix so symmetry is sort-of abandoned but not really as described by E. & G. §8.4.3, &c., &c.

So I think this PR is the right thing to do in the short term (get the example running correctly without warnings or errors or makeshifts), but it might be bettered.

I changed the behavior of solve because I think it should deal with the most general case by default or otherwise the result can be surprising: wrong results without any warning.

Yes, this was the right thing to do, for sure.

@kinnala kinnala merged commit 85df07f into kinnala:master Apr 8, 2021
@gdmcbain gdmcbain deleted the recondense-ex21 branch April 8, 2021 08:52
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

Successfully merging this pull request may close these issues.

2 participants