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

Correct numpy tolerence #167

Merged
merged 1 commit into from
May 17, 2024
Merged

Correct numpy tolerence #167

merged 1 commit into from
May 17, 2024

Conversation

atxy-blip
Copy link
Contributor

@atxy-blip atxy-blip commented May 9, 2024

In commit 254b981, atol for numpy backend was wrongly set to 1e-5 in mps/backend.py, instead of the original value 1e-8. Meanwhile, in this version only atol was set up.

def canonical_atol(self):
if self.is_32bits:
return 1e-4
else:
return 1e-5

As for numpy, the basic logic for considering two numbers a and b close in is:

abs(a - b) <= max(rtol * max(abs(a), abs(b)), atol)

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:

  • Correct the values of mps.backend.canonical_rtol and mps.backend.canonical_atol
  • Add a setter for the above properties
  • Use the tols as default parameter values in mps.matrix.check_lortho, mps.mp.check_left_canonical, mps.mp.ensure_left_canonical and their right counterparts

I am not sure whether rtol set to 1e-2 is appropriate for 32-bits. Discussions are welcome for any issue concerned : )

Copy link

codecov bot commented May 9, 2024

Codecov Report

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).

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 ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   85.87%   87.23%   +1.35%     
==========================================
  Files         117      118       +1     
  Lines       12342    13710    +1368     
==========================================
+ Hits        10599    11960    +1361     
- Misses       1743     1750       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@liwt31
Copy link
Collaborator

liwt31 commented May 9, 2024

In general the PR is OK, but I'm curious if this setting has a practical effect on the calculations you've carried out?

@atxy-blip
Copy link
Contributor Author

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 : (

@jiangtong1000
Copy link
Collaborator

jiangtong1000 commented May 9, 2024 via email

@atxy-blip
Copy link
Contributor Author

@jiangtong1000 I intend to open a new issue for discussion of the current problem. Please check #168 : D

@liwt31
Copy link
Collaborator

liwt31 commented May 10, 2024

@atxy-blip ready to merge if you fix my review comment. It'll be better if you can solve the codecov issue.

@atxy-blip atxy-blip force-pushed the fix-atol branch 2 times, most recently from 6eb51b2 to 2c37a67 Compare May 17, 2024 08:07
@atxy-blip
Copy link
Contributor Author

ready to merge if you fix my review comment

Fixed.

@liwt31
Copy link
Collaborator

liwt31 commented May 17, 2024

Thanks for the contribution! Looking forward to more of your PRs 😄

renormalizer/mps/mp.py Outdated Show resolved Hide resolved
- originally introduced in commit 254b981
@liwt31 liwt31 merged commit 1b65b1e into shuaigroup:master May 17, 2024
2 checks passed
@liwt31
Copy link
Collaborator

liwt31 commented May 17, 2024

Note that this branch has been rebased and its commits are different from your local version @atxy-blip

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.

3 participants