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

Make nk more extensible, pt 1: use dispatch in netket.hilbert.random #734

Merged
merged 5 commits into from
Jun 10, 2021

Conversation

PhilipVinc
Copy link
Member

@PhilipVinc PhilipVinc commented Jun 2, 2021

This is part 1 in making netket more easy to extend by moving to use dispatch in functions that might be extended and overridden by an user.

This PR does 2 things:

  • Use netket.utils.dispatch instead of from plum import dispatch (even if they are identical) just for the sake that if, at some point, we need to change something in the dispatch, we just need to change it in one place.
  • Use dispatch throughout netket.hilbert.random.

The next step of this PR would be to make DoubledHilbert and TensorHilbert parametric classes in order to create special dispatches (for example, if DoubledHilbert[<:HomogeneousHilbert], there is no need to split the rows and columns because they all are identical).
But I think this can be done in a second moment.

The next step would be to do the same for MCState.expect and .expect_and_grad so an user could use his own, custom operators or anything with those two functions.

@github-actions
Copy link

github-actions bot commented Jun 2, 2021

Hello and thanks for your Contribution!
I will be building previews of the updated documentation at the following link:
https://netket.github.io/netket/preview/pv/dispatch

Once the PR is closed or merged, the preview will be automatically deleted.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2021

Codecov Report

Merging #734 (60f33bd) into master (3e65b2b) will increase coverage by 0.45%.
The diff coverage is 88.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   70.80%   71.25%   +0.45%     
==========================================
  Files         230      231       +1     
  Lines       13370    13333      -37     
  Branches     1881     1870      -11     
==========================================
+ Hits         9466     9500      +34     
+ Misses       3388     3331      -57     
+ Partials      516      502      -14     
Impacted Files Coverage Δ
netket/hilbert/tensor_hilbert.py 64.65% <ø> (ø)
netket/hilbert/random/base.py 74.41% <72.97%> (+16.84%) ⬆️
netket/graph/space_group.py 100.00% <100.00%> (ø)
netket/hilbert/doubled_hilbert.py 97.14% <100.00%> (+0.04%) ⬆️
netket/hilbert/random/__init__.py 100.00% <100.00%> (ø)
netket/hilbert/random/custom.py 48.00% <100.00%> (-2.00%) ⬇️
netket/hilbert/random/doubled.py 100.00% <100.00%> (ø)
netket/hilbert/random/fock.py 59.45% <100.00%> (-2.08%) ⬇️
netket/hilbert/random/qubit.py 90.90% <100.00%> (ø)
netket/hilbert/random/spin.py 59.25% <100.00%> (-1.46%) ⬇️
... and 20 more

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 3e65b2b...60f33bd. Read the comment docs.

@PhilipVinc PhilipVinc marked this pull request as ready for review June 9, 2021 12:56
@PhilipVinc
Copy link
Member Author

This is ready to be reviewed.

It gives a stable API to override the default behaviour of random state generation.

I'd like to write a section of the manual about it, but I won't do it now.



@singledispatch
@dispatch
def flip_state_scalar(hilb, key, state, indx):
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to remove scalar and batch versions and replace them with a dispatch? or is it sci fi?

Copy link
Member Author

Choose a reason for hiding this comment

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

In a sane language yes.
In an insane language like python sometimes.

I did it for random_state because one of the inputs specifies the size of the output.
Here we should dispatch on the number of dimensions of state which is not doable yet.

Apparently it might be doable in Python 3.10. but that's still off.

I'll try to think alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

it's ok, this is still somehow an implementation detail but yeah if python 10 supports that we should go for it...!

@PhilipVinc
Copy link
Member Author

Don't merge yet because maybe i have a solution...

@PhilipVinc PhilipVinc merged commit da86ad9 into master Jun 10, 2021
@PhilipVinc PhilipVinc deleted the pv/dispatch branch June 10, 2021 15:33
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