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

fix VMC.info() error #984

Merged
merged 2 commits into from
Nov 14, 2021
Merged

fix VMC.info() error #984

merged 2 commits into from
Nov 14, 2021

Conversation

mmezic
Copy link
Contributor

@mmezic mmezic commented Nov 14, 2021

Solves #983 by renaming self.sr to self.preconditioner.

Also fixes an error in the function info(obj, depth=None) when obj has an attribute info which is not a method.

Copy link
Member

@PhilipVinc PhilipVinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Though I think we should change repr to give more information and get rid of this info...

But that's a good stopgap

@PhilipVinc
Copy link
Member

I did not know that hasattr syntax! Nice

Copy link
Collaborator

@femtobit femtobit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, VMC.info() is a bit of a leftover from v2. But this fix is nice to have for now, thanks @mmezic!

@PhilipVinc
Copy link
Member

Out of curiosity, @mmezic , do you actually use info() ? For what?

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2021

Codecov Report

Merging #984 (851f83d) into master (a9da8f2) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #984   +/-   ##
=======================================
  Coverage   80.14%   80.14%           
=======================================
  Files         172      172           
  Lines       10448    10448           
  Branches     1490     1490           
=======================================
  Hits         8374     8374           
  Misses       1677     1677           
  Partials      397      397           
Impacted Files Coverage Δ
netket/driver/vmc.py 89.79% <ø> (ø)
netket/driver/vmc_common.py 25.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9da8f2...851f83d. Read the comment docs.

Copy link
Collaborator

@femtobit femtobit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know that hasattr syntax! Nice

Actually, that doesn't work as far as I can tell:

>>> class VMC: 
...     def info(self):
...         print("info")                                                                                                                  
...
>>> vmc = VMC()                                                                                                           
>>> vmc.info()                                                                                                            
info
>>> hasattr(vmc, "info")                                                                                                  
True
>>> hasattr(vmc, "info()")                                                                                               
False

This should work better:

>>> hasattr(vmc, "info") and callable(vmc.info)
True

netket/driver/vmc_common.py Outdated Show resolved Hide resolved
@mmezic
Copy link
Contributor Author

mmezic commented Nov 14, 2021

Out of curiosity, @mmezic , do you actually use info() ? For what?

I came across info() while reading the documentation and testing a few things. I don't need this function, so you can probably remove it. I was just surprised that it causes the error by default.

@femtobit
Copy link
Collaborator

femtobit commented Nov 14, 2021

[...] because preconditioner has an attribute "info", which is not callable. Your change fixes it.

Well, our test coverage of this method is obviously lacking. The fact that info is an attribute on the preconditioner is probably also unintended. Good that you spotted those.

I still think we should still fix the method on VMC in this PR, but maybe deprecate it in the future. @PhilipVinc ?

@PhilipVinc
Copy link
Member

What fix do we need?

I'm in Favour of deprecating.
Actually I've long wanted to transition to Jupiter's pretty print methods for this interactive stuff

@femtobit femtobit merged commit 2023871 into netket:master Nov 14, 2021
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.

4 participants