Gp/continuous geometry #1583

gpescia · 2023-09-20T15:25:37Z

First draft to implement the geometry for continuous space.
There is continous_geometry.py as base class for specialized ones: Cell, Free.
At the moment only these two are available (so no mix of boundary conditions).
I commented out all tests in hilbert that test for mixed boundary conditions as well as the Langevin sampler.
Examples run fine on my machine.

This reverts commit f734d0e.

…nly two classes are implemented: Cell = fully periodic, Free = no pbc at all. Commented out tests with mixed boundary conditions. Commented out tests of Langevin sampler

netket/hilbert/continuous_hilbert.py

PhilipVinc · 2023-09-20T15:39:10Z

We should decide where to put the geometry stuff. I'm not really in favour of .graph let's see what @gcarleo thinks about
We should not break old interfaces. So ContinuousHilbert should still accept the old arguments, and also geometry. If geometry is none it should use the other arguments to build a geomtry
This way of implementing the random states is not ok

Before actually implementing this stuff, I think someone should lay out clearly what we want to do with it. For example, do we need the inverse lattice? Why? What is the expected use case of this?

For example, the messy translation code in the sampling makes me suspect that the abstract class should have a method translate_positions_by_vector or something like that...

gpescia · 2023-09-20T15:43:00Z

We should not break old interfaces. So ContinuousHilbert should still accept the old arguments, and also geometry. If geometry is none it should use the other arguments to build a geomtry

yes true, I‘ll add the other as well.

This way of implementing the random states is not ok

In the end I guess we want to dispatch on cell/free.

For example, do we need the inverse lattice? Why? What is the expected use case of this?

yes to go from lattice basis to standard basis and back.

PhilipVinc · 2023-09-20T15:45:00Z

yes to go from lattice basis to standard basis and back.

In what part of the code is this needed? If this is not used in the code, can you give an example of what it can be used for and how it would be used?

gcarleo · 2023-09-20T15:45:40Z

Yes for the position of the module, it's a secondary problem, but I also wouldn't put it in Graph. Anyways, there is time to think about that...

In my view the Geometry object should have a minimal interface to start with allowing to:

Compute distances between any two points R1,R2 that are valid for the given Geometry. This method is just distance and should know nothing about periodicity etc explicitly (i.e. you don't pass pbc=True etc to distance) it is the essence of the Geometry to know how the distance should be computed (e.g. with minimum image etc)
Generate a valid random point R
Add/subtract vectors in this space (say implement R_1 + R_2)? (not sure about this point but maybe it's handy for sampling)

gpescia · 2023-09-20T15:46:41Z

In the sampler and the hilbert space random state. We sample fractional coordinates, for this we need to go from standard to lattice basis back to standard

gcarleo · 2023-09-20T15:47:24Z

In the sampler and the hilbert space random state. We sample fractional coordinates, for this we need to go from standard to lattice basis back to standard

but that's why the random points should be generated in the Geometry, not in Hilbert, see point 2 of the list I made above. I think this allows to simplify most of the code used for sampling

gpescia · 2023-09-20T15:49:34Z

Well you need to tell the function whether you want periodic in the cell or minimum image convention. For this you need the ‚mode’ I would say. In the free case you don‘t need it.
The only difference is the placement of the random state right? One still goes nack and forth between the two bases.

gcarleo · 2023-09-20T15:51:31Z

Yes but you specify periodic or minimum image when you construct Cell, not when you call Distance, in this way you completely separate the sampling and Hilbert from these details of the geometry
It's important to disentangle everything that relates to Geometry from Hilbert and the Sampling, so that you have a general code for the last two that can work for any Geometry

PhilipVinc · 2023-09-20T15:52:29Z

Well you need to tell the function whether you want periodic in the cell or minimum image convention. For this you need the ‚mode’ I would say. In the free case you don‘t need it.

Are there cases where for the same geometry one wants one of those two different distances? Can you give an example?
Can we think a case of a default distance to be used mostly everywhere? In that case Geometry objects could expose different distance functions, and a default one to be used for interaction potentials or I don't know what else.

gcarleo · 2023-09-20T15:54:44Z

I think there is no reasonable case in which you want to use two different distances within the same simulation Cell, that's why I think the Cell type should be used to identify uniquely the type of distance used as well

gpescia · 2023-09-20T15:55:55Z

Well potential energies are computed always with minimum image, and distances for the wf never. So you need both…

PhilipVinc · 2023-09-20T15:56:57Z

What do you mean by distances for the wf ?

codecov · 2023-09-20T15:57:11Z

Codecov Report

Attention: 60 lines in your changes are missing coverage. Please review.

Comparison is base (ee98fef) 83.53% compared to head (a88a945) 84.22%.
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1583      +/-   ##
==========================================
+ Coverage   83.53%   84.22%   +0.69%     
==========================================
  Files         257      261       +4     
  Lines       14519    15155     +636     
  Branches     2203     2232      +29     
==========================================
+ Hits        12129    12765     +636     
- Misses       1841     1852      +11     
+ Partials      549      538      -11

Files	Coverage Δ
netket/experimental/__init__.py	`100.00% <100.00%> (ø)`
netket/experimental/geometry/__init__.py	`100.00% <100.00%> (ø)`
netket/hilbert/continuous_hilbert.py	`100.00% <100.00%> (+22.22%)`	⬆️
netket/hilbert/random/particle.py	`84.21% <100.00%> (-15.79%)`	⬇️
netket/models/deepset.py	`94.44% <100.00%> (+1.11%)`	⬆️
netket/operator/_kinetic.py	`94.82% <100.00%> (ø)`
netket/sampler/rules/continuous_gaussian.py	`89.47% <100.00%> (-1.44%)`	⬇️
...etket/experimental/geometry/continuous_geometry.py	`83.33% <83.33%> (ø)`
netket/hilbert/particle.py	`87.03% <78.78%> (-10.47%)`	⬇️
netket/experimental/geometry/Free.py	`53.65% <53.65%> (ø)`
... and 1 more

... and 76 files with indirect coverage changes

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

gpescia · 2023-09-20T15:57:32Z

It's important to disentangle everything that relates to Geometry from Hilbert and the Sampling, so that you have a general code for the last two that can work for any Geometry

But random state is actually a state in hilbert not in the geometry, right?

gpescia · 2023-09-20T15:58:07Z

distance features used in the wavefunction

gcarleo · 2023-09-20T16:02:35Z

sorry Gabriel but is it truly necessary to use two different distances? I would say one should be enough / is much nicer than using two different distances for the same space

gcarleo · 2023-09-20T16:04:21Z

in any case, maybe it's more efficient to talk on zoom tomorrow or so

gpescia · 2023-09-20T16:06:32Z

Sure we can have a meeting! I‘m just saying that there are two distance measures which both get used. I‘m not sure how to optimally resolve that..

PhilipVinc · 2023-09-20T16:09:09Z

I would love to understand why you are using more than one distance.
If I understand it's because in the nn/variational ansatz you want to use the standard distance instead of the minimum image.
But physically this, I think, is wrong, because the minimum image is the physical one in a periodic system.
So what is the underlying reason of this? Performance? It works? Or maybe I fail to understand?

gpescia · 2023-09-20T16:11:25Z

The reason is smootheness. The minimum image convention reproduces Euclidean distance as you say but it is not smooth and cannot be used as input to the wavefunction as we take derivatives w.r.t. position. The other distance (sin-distance) behaves similar as the minimum image distance but is smooth. So for potential energies (no derivatives involved) we want the "correct" euclidean distance. For the wavefunction we take the less correct but smooth version.

PhilipVinc · 2023-09-20T16:13:34Z

Thanks! This was the explanation I was looking for.
I think that to maintain flexibility we could should just implement both as two different functions. Then in the models, one uses what he wants.
But to be further discussed..

gcarleo · 2023-09-20T16:14:14Z

OK, not ideal, but then this is solved maybe specifying what type of distance you want, it's true that even in euclidean space you might want L1,L2... etc

gpescia · 2023-09-20T16:14:43Z

sure we can do that too. But that is basically the same as setting a 'mode' flag or do you gain performance with this?

gcarleo · 2023-09-20T16:15:20Z

Yes I think it's just a flag to be passed: geo.Distance(R1,R2,type='L2') etc

PhilipVinc · 2023-09-20T16:17:00Z

It's not about performance. It's about having a clean and sane API.
I think geometry.distance should always give the physical distance, aka minimum image.
In some cases you might want to cheat / use non physical distances in which case those should have different names.
mode might hide them a bit.
But I'm not completely convinced.
Maybe mode is good.
Would like to hear opinions.

gcarleo · 2023-09-20T16:19:10Z

if you want the main problem is where the type flag is to be used. I think for the ansatz there is no problem since you will have an ansatz specific to periodic systems that will also know about different types of distance that a Cell can accept. Now we have to understand how we make the sampler agnostic somehow of the type of Geometry

@PhilipVinc the physical distance in a sense is not well defined, it might be that it is L1 that makes sense and not L2, depending on the problem (just to give an example). In the same spirit for the periodic system one type might make more sense than the other type of distance...

gpescia · 2023-09-20T16:52:48Z

Ah seems it falls back on the system version of black rather than the one of the environment when running it as a script.
python -m black {source_file_or_directory} works.

…ostic to hilbert. Distance functions for free and periodic space. Hash does not yet work. Langevin not yet working.

gpescia · 2023-09-21T14:35:00Z

Does it destroy back-compatibility if we error out when mixed boundary conditions are used? I have never seen boundary conditions in one direction but not in the other ones in the context of continuous space. The only place this gets used is netket tests. I would be very much in favour for removing this.

gcarleo · 2023-09-21T14:36:23Z

I am not sure it's necessary to keep back compatible interfaces though, this is all in netket.experimental, part of the deal is that we can break these interfaces without major releases

PhilipVinc · 2023-09-21T14:38:23Z

This is not in netket.experimental. We could break a bit, but for a good reason (and I'm not sure that creating a geometry is necessary all the time)

gpescia · 2023-09-21T14:41:56Z

I just think that supporting mixed boundary conditions makes it more complicated to take care of all cases while not providing any benefit in terms of usability. If you really want mixed boundary conditions just take pbc everywhere and make one dimension very large...
But that is up to discussion I guess.

Edit: Also I have no idea what would be the correct distance in this case and how to correctly do the periodic decomposition.

PhilipVinc · 2023-09-21T15:19:14Z

I'm not against dropping support for mixed boundary conditions, though due to MPS it's a not-so-uncommon scenario.
It could raise a not implemented error for now.

PhilipVinc

I would suggest that we agree on the minimal interface of the ÀbstractGeomtry object before going forward to avoid wasting time and effort...
The interface means defining every attribute and method name with its signature.
Once this is defined, it can be implemented.

My proposal is

class AbstractGeometry(abc.ABC):
   
   @property
   def n_dimensions(self) -> int:
        # the number of physical dimensions

    def distance(self, x: Array, y: Array) -> Array:
         # computes the physical distance. not necessarily smooth or differentiable.

     def translate(self, x: Array, R : Array) -> Array:
         # translates configuration x by vector R

     def random_uniform(self, shape: Union[int, Tuple[int, ...]], dtype: DType) -> Array:
          # random configurations uniform distribution

We still need to:

decide how to compute other distances. Should it be a mode=... keyword argument or a different function? if it's a mode, how should it be handled by spaces that don't support it?
- Does this mean that we must implement the same mode everywhere? What should it be called the mode that gives the L2 distance (not minimum image)?
If we decide not to use mode how do we write general nn models that work with arbitrary geometries? should we add a distance_differentiable(x,y) method to this abstract class?
Positioning of this stuff. I propose adding the namespace netket.experimental.geometry for the time being and placing this stuff there. Eventually we can move this to netket.geometry.

@gpescia should confirm if he believes this minimal interface is enough to do everything we need.

Then, Cell and FreeSpace can add new properties and eventually new methods that are useful.

What is the Latticeargument that Gabriel is using? what does it represent?

netket/graph/continuous_geometry.py

PhilipVinc · 2023-09-21T15:46:08Z

netket/graph/continuous_geometry.py

+        r"""Spatial extension in each spatial dimension"""
+        raise NotImplementedError
+
+    def distance(self, x, y=None, norm=False, mode=None) -> list[list]:


why y=None? what does it mean? is it needed? what is norm?

indeed what is norm? mode should be enough I guess (e.g. mode=L2 )

If y=None it means distance(x,x) with the caveat that the norm is computed differently in that case.

To not having to call the method twice in case you want the norm and the vectors, there is the extra flag norm. We could have a seperate mode keyword for that.

I don't understand. The definition of a distance is such that distance(x,x) is zero.

not if x contains N different positions. x can have shape (nsamples, nparticles, sdim) then only the diagonal is 0.

So in the case of x being a batch of samples, you return a matrix nsamples x nsamples of distances?

Can't we do this by having the following signature for the distance function?

distance( x:Array[N], y: Array[N]) -> Array[] # (a scalar)

distance( x:Array[B1, N], y: Array[N]) -> Array[B1]

distance( x:Array[N], y: Array[B2,N]) -> Array[B2]

distance( x:Array[B1, N], y: Array[B2,N]) -> Array[B1, B2]

distance( x:Array[B1, B2, N], y: Array[B3, N]) -> error

(nsamples, nparticles, nparticles, sdim) or the upper triangular in case specified (nsamples, nparticles*(nparticles-1)/2, sdim).

Now the function works for x=(…,N,sdim) and y=(…,M,sdim) but yes we could use dispatch.

netket/graph/continuous_geometry.py

PhilipVinc · 2023-09-21T15:48:21Z

netket/graph/continuous_geometry.py

+        put between 0 and 1."""
+        raise NotImplementedError
+
+    def from_lat_to_standard(self, x):


can we agree on the name? I don't like the arbitrary abbreviation. I don't understand what standard means. Where is this used? Is it necessary to have it part of the base class?

yes this should not be in the base class it's a specific thing of Cell I guess

I think it is a naming issue. What lattice is supposed to be is a basis of the physical space. Standard is the standard basis e.g. (1.0),(0,1) in 2D. So a better naming instead of lattice is probably basis. The two functions then allow to transform back and forth between the two bases.
I actually think it belongs to the base class. I‘m not sure of a use case right now but also in free space it might make sense to define another basis than the standard basis.

we should try to use terminology that is also consistent with Graph.Lattice (actually that class contains most of the periodic cell functionalities already, so there might be overlapping code we can reuse?)

netket/graph/continuous_geometry.py

gcarleo · 2023-09-21T16:49:06Z

I agree we should first settle on an Abstract Geometry class. I think what @PhilipVinc is ok, but I would make some changes (mostly, I think translate is too specific, the general thing is adding vectors or performing other operations)

class AbstractGeometry(abc.ABC):
   
   @property
   def n_dimensions(self) -> int:
        # the number of physical dimensions

    def distance(self, x: Array, y: Array, mode=None) -> Array:
         # computes the physical distance. not necessarily smooth or differentiable.
         # every geometry implements different modes that are specific of that geometry

    def  add(self, A: Array, B: Array): -> Array:
         # A+B with folding into pbc if prescribed by the geometry etc 

     def random_uniform(self, shape: Union[int, Tuple[int, ...]], dtype: DType) -> Array:
          # random configurations uniform distribution

I also endorse netket.experimental.geometry for the time being

PhilipVinc · 2023-09-21T16:51:08Z

def __add__(self, A: Array, B: Array): -> Array: is to implement addition of the object itself, namely geometry, not two third party objects,

gcarleo · 2023-09-21T16:52:45Z

yes, ok... indeed I think the main issue is that a vector should not be a vector but a Geometry object actually

gcarleo · 2023-09-21T16:53:26Z

or, better, a Geomety has Elements that are what you are calling Array now

gcarleo · 2023-09-21T16:54:34Z

or, much easier:

def add(self,A,B) this seems legitimate to me. You just call geo.add(R1,R2)

PhilipVinc · 2023-09-21T16:54:41Z

Yes, a PointNd, which is generated by the Geometry object.
But let's not get into such details because a custom object would break all of jax and let's implement Geometry.add(x, y), what do you think?

PhilipVinc · 2023-09-21T16:55:34Z

Ok, what about the modeargument to distance? what should be the default? what are other possible values we reasonably can think about?

gcarleo · 2023-09-21T17:41:04Z

It might be good also to check what other establidhed libraries do, I found this for example https://wiki.fysik.dtu.dk/ase/ase/atoms.html#ase.Atoms.get_distance

PhilipVinc · 2023-09-25T17:09:49Z

@gcarleo one more thing that comes to my mind: geometry could be useful (think necessary) to specify the partition one wants to compute entanglement entropy for in a continuous system.

It would be useful to think about how to handle/expose specifying a sub-part of the system there.

…eometry. Mixed boundary conditions throw error.

gcarleo · 2023-09-26T15:25:58Z

netket/experimental/geometry/continuous_geometry.py

+        positions back to the geometry defined."""
+
+    @abc.abstractmethod
+    def random_init(self, shape):


I wouldn't call this random_init, rather just random_uniform or similar? (the init part is not relevant from the Geometry point of view)

For free space I just return zeros, so it is also not really uniform (also not random actually).. So both names are a bit misleading maybe

I had proposed random_defaulta while back

gcarleo · 2023-09-26T15:26:33Z

netket/experimental/geometry/continuous_geometry.py

+        For free space this could just return zeros. For periodic space one could initialize on a lattice.
+        N: int, number of particles."""
+
+    def from_basis_to_standard(self, x):


I am not sure these belong to the general interface, for example they seem to be used only by Cell?

For now yes, but I think the defining property of a geometry is a basis, and there might be instances where you want to define a different basis than the standard basis also for free space. Then going back and forth between the custom basis and the standard basis should be part of geometry in my view.

gcarleo · 2023-09-26T15:28:14Z

netket/hilbert/particle.py

-            pbc: Tuple or bool indicating whether to use periodic boundary conditions in a given physical dimension.
-                If tuple it must have the same length as domain. If bool the same value is used for all the dimensions
-                defined in domain.
+            pbc: Tuple of bools or bool indicating whether to use periodic boundary conditions in a given physical


personally, I would be in favor of breaking the interface instead of adding deprecations and convoluted consutrctors etc... but OK, it's not so user friendly maybe :)

gpescia added 3 commits September 19, 2023 16:55

Revert "change naming from mass to inverse_mass in kinetic energy"

f6916fc

This reverts commit f734d0e.

First implementation of geometry for continous space. At the moment o…

e7559ee

…nly two classes are implemented: Cell = fully periodic, Free = no pbc at all. Commented out tests with mixed boundary conditions. Commented out tests of Langevin sampler

formatting

0483441

gcarleo reviewed Sep 20, 2023

View reviewed changes

netket/hilbert/continuous_hilbert.py Outdated Show resolved Hide resolved

formatting2

328ec6c

Backcompatibility of hilbert and sampler restored. Sampler is now agn…

c48da8a

…ostic to hilbert. Distance functions for free and periodic space. Hash does not yet work. Langevin not yet working.

add __eg__

78bbc31

PhilipVinc reviewed Sep 21, 2023

View reviewed changes

gpescia added 2 commits September 26, 2023 16:56

Added geometry in netket.experimental. The hilbert now constructs a g…

b1418b6

…eometry. Mixed boundary conditions throw error.

typo in HO.py

fafd0f7

gcarleo reviewed Sep 26, 2023

View reviewed changes

fix import

a88a945

PhilipVinc force-pushed the master branch 3 times, most recently from 562101f to 5d8bad1 Compare October 23, 2023 20:38

PhilipVinc force-pushed the master branch from 31404c9 to f0297a5 Compare November 19, 2023 06:16

gpescia mentioned this pull request Apr 9, 2024

[FR] Euclidian distance between nodes in nk.graph.lattice #1682

Open

Gp/continuous geometry #1583

Are you sure you want to change the base?

Gp/continuous geometry #1583

Conversation

gpescia commented Sep 20, 2023

PhilipVinc commented Sep 20, 2023

gpescia commented Sep 20, 2023

PhilipVinc commented Sep 20, 2023

gcarleo commented Sep 20, 2023 • edited

gpescia commented Sep 20, 2023

gcarleo commented Sep 20, 2023 • edited

gpescia commented Sep 20, 2023

gcarleo commented Sep 20, 2023

PhilipVinc commented Sep 20, 2023

gcarleo commented Sep 20, 2023

gpescia commented Sep 20, 2023

PhilipVinc commented Sep 20, 2023

codecov bot commented Sep 20, 2023 • edited

Codecov Report

gpescia commented Sep 20, 2023

gpescia commented Sep 20, 2023

gcarleo commented Sep 20, 2023

gcarleo commented Sep 20, 2023

gpescia commented Sep 20, 2023

PhilipVinc commented Sep 20, 2023

gpescia commented Sep 20, 2023 • edited

PhilipVinc commented Sep 20, 2023

gcarleo commented Sep 20, 2023

gpescia commented Sep 20, 2023

gcarleo commented Sep 20, 2023

PhilipVinc commented Sep 20, 2023

gcarleo commented Sep 20, 2023

gpescia commented Sep 20, 2023

gpescia commented Sep 21, 2023

gcarleo commented Sep 21, 2023

PhilipVinc commented Sep 21, 2023

gpescia commented Sep 21, 2023 • edited

PhilipVinc commented Sep 21, 2023

PhilipVinc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpescia Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcarleo commented Sep 21, 2023 • edited

PhilipVinc commented Sep 21, 2023

gcarleo commented Sep 21, 2023

gcarleo commented Sep 21, 2023

gcarleo commented Sep 21, 2023

PhilipVinc commented Sep 21, 2023

PhilipVinc commented Sep 21, 2023

gcarleo commented Sep 21, 2023

PhilipVinc commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcarleo commented Sep 20, 2023 •

edited

gcarleo commented Sep 20, 2023 •

edited

codecov bot commented Sep 20, 2023 •

edited

gpescia commented Sep 20, 2023 •

edited

gpescia commented Sep 21, 2023 •

edited

gpescia Sep 21, 2023 •

edited

gcarleo commented Sep 21, 2023 •

edited