Closed atxy-blip closed 6 months ago
Attention: Patch coverage is 94.54545%
with 3 lines
in your changes are missing coverage. Please review.
Project coverage is 87.23%. Comparing base (
07b639f
) to head (2c37a67
).:exclamation: Current head 2c37a67 differs from pull request most recent head 24626e1
Please upload reports for the commit 24626e1 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
renormalizer/mps/mp.py | 85.00% | 3 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
In general the PR is OK, but I'm curious if this setting has a practical effect on the calculations you've carried out?
if this setting has a practical effect on the calculations you've carried out
Nope. Code from the lastest master branch still messes up our Holstein test after I set $RENO_NUM_THREADS
to 1 and appied the new atol. I am still investigating on why it happens on my setup.
Anyway, proposing a fix on atol is the least thing I can do at this stage : (
What do you mean by messing up?
On Thu, May 9, 2024 at 6:03 AM Yu Xiong @.***> wrote:
if this setting has a practical effect on the calculations you've carried out
Nope. Code from the lastest master branch still messes up our Holstein test after I set $RENO_NUM_THREADS to 1 and appied the new atol. I am still investigating on why it happens on my setup.
Anyway, proposing a fix on atol is the least thing I can do at this stage : (
— Reply to this email directly, view it on GitHub https://github.com/shuaigroup/Renormalizer/pull/167#issuecomment-2102352484, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZLWMJW2PXQ7FUFOAVL7ZLZBNCXBAVCNFSM6AAAAABHONXYD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBSGM2TENBYGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
@jiangtong1000 I intend to open a new issue for discussion of the current problem. Please check #168 : D
@atxy-blip ready to merge if you fix my review comment. It'll be better if you can solve the codecov issue.
ready to merge if you fix my review comment
Fixed.
Thanks for the contribution! Looking forward to more of your PRs 😄
Note that this branch has been rebased and its commits are different from your local version @atxy-blip
In commit 254b981,
atol
for numpy backend was wrongly set to1e-5
inmps/backend.py
, instead of the original value1e-8
. Meanwhile, in this version onlyatol
was set up.https://github.com/shuaigroup/Renormalizer/blob/254b9819d0e9124736d9c1681de0ec7bc60792bb/renormalizer/mps/backend.py#L136-L140
As for
numpy
, the basic logic for considering two numbersa
andb
close in is:The existence of
rtol
allows for the comparison of large numbers, e.g., 10001 and 10000, which may emerge in imaginary time-evolution. Therefore, I propose adding back the rtol parameter.This commit does the following things:
mps.backend.canonical_rtol
andmps.backend.canonical_atol
mps.matrix.check_lortho
,mps.mp.check_left_canonical
,mps.mp.ensure_left_canonical
and their right counterpartsI am not sure whether
rtol
set to 1e-2 is appropriate for 32-bits. Discussions are welcome for any issue concerned : )