-
Notifications
You must be signed in to change notification settings - Fork 174
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
Gp/continuous geometry #1583
base: master
Are you sure you want to change the base?
Gp/continuous geometry #1583
Conversation
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
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 |
yes true, I‘ll add the other as well.
In the end I guess we want to dispatch on cell/free.
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? |
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:
|
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 |
|
|
Are there cases where for the same geometry one wants one of those two different distances? Can you give an example? |
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 |
Well potential energies are computed always with minimum image, and distances for the wf never. So you need both… |
What do you mean by |
Codecov ReportAttention:
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
☔ View full report in Codecov by Sentry. |
But random state is actually a state in hilbert not in the geometry, right? |
distance features used in the wavefunction |
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 |
in any case, maybe it's more efficient to talk on zoom tomorrow or so |
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.. |
I would love to understand why you are using more than one distance. |
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. |
Thanks! This was the explanation I was looking for. |
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 |
sure we can do that too. But that is basically the same as setting a 'mode' flag or do you gain performance with this? |
Yes I think it's just a flag to be passed: |
It's not about performance. It's about having a clean and sane API. |
if you want the main problem is where the @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... |
Ah seems it falls back on the system version of black rather than the one of the environment when running it as a script. |
…ostic to hilbert. Distance functions for free and periodic space. Hash does not yet work. Langevin not yet working.
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. |
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 |
This is not in |
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... Edit: Also I have no idea what would be the correct distance in this case and how to correctly do the periodic decomposition. |
I'm not against dropping support for mixed boundary conditions, though due to MPS it's a not-so-uncommon scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 themode
that gives the L2 distance (not minimum image)?
- Does this mean that we must implement the same
- If we decide not to use
mode
how do we write general nn models that work with arbitrary geometries? should we add adistance_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 tonetket.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 Lattice
argument that Gabriel is using? what does it represent?
netket/graph/continuous_geometry.py
Outdated
r"""Spatial extension in each spatial dimension""" | ||
raise NotImplementedError | ||
|
||
def distance(self, x, y=None, norm=False, mode=None) -> list[list]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why y=None? what does it mean? is it needed? what is norm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed what is norm? mode
should be enough I guess (e.g. mode=L2
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. The definition of a distance is such that distance(x,x)
is zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not if x contains N different positions. x can have shape (nsamples, nparticles, sdim) then only the diagonal is 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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
Outdated
put between 0 and 1.""" | ||
raise NotImplementedError | ||
|
||
def from_lat_to_standard(self, x): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this should not be in the base class it's a specific thing of Cell
I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
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 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 |
|
yes, ok... indeed I think the main issue is that a vector should not be a vector but a |
or, better, a |
or, much easier:
|
Yes, a |
Ok, what about the |
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 |
@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.
positions back to the geometry defined.""" | ||
|
||
@abc.abstractmethod | ||
def random_init(self, shape): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had proposed random_default
a while back
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure these belong to the general interface, for example they seem to be used only by Cell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
562101f
to
5d8bad1
Compare
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.