-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add tolerance to krylov_schur
#3
Conversation
There are two other things that I don't really know where to discuss because I can't open issues here, so I will just mention them here.
comparing
|
Sounds fine in principle, but does
First of all: What is ill-conditioned here (how du you caclulate it and what magnitude do you get?)?
|
@marius have you tried dimension reduction before generating your transition matrix (e.g. via PCA or ICA and kmeans clustering or some more advanced clustering technique)? |
Hi Bernhard, thanks for getting back to me. There are quite a few things here so I would say let's discuss all of them during our skype tomorrow. The easy change I can do right now is to change the tolerance used by krylov-schur to be 1e-16 by default. |
Just to remind myself: If msmtools/msmtools/util/sorted_schur.py Line 489 in 914c0dd
the result is compared with the original schur vectors from msmtools/msmtools/util/sorted_schur.py Line 486 in 914c0dd
to ensure that we are still in the invariant subspace... |
Right, if the method is
and this doesn't tell me anything about cutting through a block of complex conjugetes being split. |
@msmdev , I have changed this PR to no longer expose the tolerance to the user but instead to change it to 1e-16 internally. |
Hi @msmdev , can we merge this or would you like me to change anything? |
Mb. you could change the sorting criterion in |
No, you are right, I will change this here as well! I will also remove the smallest ev calculation then as that's no longer needed if we change the sorting criterion. |
Krylov schur
Hi @msmdev , what's the sorting used in 'brandts'? Is it just distance from the unit circle, or distance from a target value, e.g. 1? |
@Marius1311 Currently you can choose between "'LM': the m eigenvalues with the largest magnitude are sorted up." and "'LR': the m eigenvalues with the largest real part are sorted up." msmtools/msmtools/util/sort_real_schur.py Line 263 in 914c0dd
|
Thanks! Could it be that this does not work in 100% of the cases? I'm checking this in an example case by taking the full output of brandts, i.e. matrix R and Q, the full, sorted real schur decomposition. I'm then turning this into a complex schur decomposition using First, I compute the real schur decomposition:
Then, I need two utility functions to check the sorting:
I run these:
My output is:
|
Would you say that counts as numerical inprecision? Would you expect the sorting to be perfect? It get's the first 20 eigenvalues right, which is something I guess. |
This is quite off from the behavior I had with the original Brandts method in Matlab. Markedly, the problem occurs (in the above example) only with complex conjugate pairs. This is interesting, but I can't reasonably explain it. |
Do you have comparable issues with Krylov-Schur? |
I haven't checked this with krylov-schur yet, but will do. |
Update from my sidemethod
|
I pushed a further change here: using eigenvectors rather than the backward iteration for the stationary distribution. In my case, this reduces the total computation time by a factor of 20, from 11 minutes to 30 seconds. |
This makes the implementation practical for really large matrices. I'm just trying it on a 30k x 30k matrix and it run in 30 seconds for three states, 39 seconds for 5 states, all on my laptop. |
That's wonderful news!
Perfect!
I think this is acceptable considering the use of
Matches with my troubles with this method. It's sad, since I planed to replace
I agree - lets do it so :) |
Co-authored-by: Bernhard Reuter <[email protected]>
Co-authored-by: Bernhard Reuter <[email protected]>
Co-authored-by: Bernhard Reuter <[email protected]>
Okay cool, I will go ahead then and remove scipy for sorting. |
These latest commits remove I tested this version on an example and it works. |
Hi @msmdev , from my side, everything is ready for merging. Is there anything else from your side? |
Please excuse the delay - I'm currently quite sick (strong headaches), but I promise to tend to it this very day. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its all fine, except I'm somehow insecure regarding the changes in stationary_vector
(see comments)...
The latest commit adds the three checks we discussed above. |
I tested this already. Initially, the thresholds for the tests were too stringent so I relaxed them a bit. It works fine now. |
The lastest commit increases the tolerance for the irreducibility check and sorts the top 2 computed vectors by their real part since I just realised in an example that scipy does not always return them in the right order. With all of this included, I reliably get the stationary distribution on a 32k x 32k sparse, ill-cond matrix in <4s on my laptop. |
@msmdev , anything missing or can we merge? |
This adds the possibility to change the convergence threshold used internally by SLEPc. Default is 1e-8, which is SLEPc's default. It also relaxes the tolerance used for checking whether the rows of the memberships vectors sum to one.
This solves my problems with
krylov_schur
not returning invariant subspaces for ill-conditioned matrices.