-
Notifications
You must be signed in to change notification settings - Fork 12
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
Make sdiag_inv_sqrt
use pseudo-inverse
#78
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
I'm not super sure about the default inverse cutoff tolerance to choose here; any opinions on this? @lkdvos |
I think TensorKit uses a |
I like that TensorKit way of handling the singular value cutoff tolerance. I have decided to again cut the |
Also updated the p-wave tests a bit and again found it weird that apparently the p-wave differentiation in |
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.
Looks good to me. If you checked the gauge sensitivity again after the fixes, Im okay with merging this like this.
@@ -36,8 +31,8 @@ gradmodes = [ | |||
LinSolver(; solver=KrylovKit.GMRES(; tol=gradtol), iterscheme=:fixed), | |||
LinSolver(; solver=KrylovKit.GMRES(; tol=gradtol), iterscheme=:diffgauge), | |||
], | |||
[ | |||
nothing, | |||
[ # Only use :diffgauge due to high gauge-sensitivity (perhaps due to small χenv?) |
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.
did you re check this after the gaugefixing fixes?
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.
Okay I checked again and the gauge-fixing fixes don't make a difference to the high gauge-sensitivity. I will have to investigate and fix that in a different PR. I just haven't played around that much with fermions yet.
I put back in nothing
to the test, since I accidentally left that commented out as I was trying around.
ctm_alg = CTMRG(; | ||
tol=1e-8, | ||
maxiter=150, | ||
verbosity=2, | ||
ctmrgscheme=:simultaneous, | ||
svd_alg=SVDAdjoint(; rrule_alg=Arnoldi(; tol=1e-9, krylovdim=χenv + 30)), | ||
) |
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.
at the risk of sounding like a broken record, I really don't like that we have to change the settings of all algorithms on every single update of the package. We really have to get some better defaults in, because this is not really feasible in the long run. (not something that has to be fixed in this pr)
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.
You definitely have a point in repeating that and I completely agree :-) Actually, for most of the tests we should already be able to replace all settings with the default ones I just haven't gotten around to change them. I just thought that I would wait until we anyway redesign our interface and code up some kind of default parameter selection thing.
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.
OK if tests turn green.
Implements pseudo-inverse for the inverse square root inside of
sdiag_inv_sqrt
and its reverse-rule.Closes #73 as suggested by @huangrzh.