WIP: Replace shared_ptr with unique_ptr in Hamiltonian and Hilbert
Created by: femtobit
Continuing the discussion from https://github.com/netket/netket/pull/39#discussion_r200202647:
I'd suggest changing the shared pointers in Hamiltonian
and Hilbert
back to unique_pr
. This will, however, force us to think a bit more carefully about ownership, specifically whether the Hamiltonian should own the Hilbert object. (This PR is still work-in-progress and does not even compile yet. It is just meant to be a basis for discussion for now, the main points are in this comment.)
As far as I can see, in Hamiltonian
the h_
pointer never even leaves the class, so I do not see the need of having a shared pointer.
For Hilbert
the case is a bit more complicated. (I guess this is why the shared pointers might have been introduced?) Using a unique pointer prevents simply copying Hilbert
, which is why I have removed the copy-constructor here: https://github.com/netket/netket/blob/2aa9c6f99eeeb843c05b5d83fc5cb5bacdcb1c09/NetKet/Hilbert/hilbert.hpp#L37
This causes the tests to no longer build, because of the following code (and similar lines below) which currently requires copying the shared pointer: https://github.com/netket/netket/blob/2aa9c6f99eeeb843c05b5d83fc5cb5bacdcb1c09/Test/Hilbert/unit-hilbert.cc#L40-L48
As far as I can see, the issue is the following: There are to ways to construct Hilbert
instances that are used in this test. One is to construct the object directly from a Hilbert
section in the JSON. The other is to create a Hamiltonian from JSON and then extract its Hilbert
object, discarding the Hamiltonian afterwards
In the first case Hamiltonian
does not own the Hilbert
object (actually there is no Hamiltonian) but it is owned by the test code. This works fine with unique_ptr
.
In the second case, the ownership of the Hilbert object lies with the Hamiltonian
class, in which case the test code has to construct the Hamiltonian and then obtain a reference to Hilbert from it. In the current code, Hamiltonian
and the test case functions share ownership of the Hilbert space via the shared pointer (and the Hamiltonian
object does not live as long as Hilbert
here).
In order to resolve this problem (which only affects the test case at the moment, if I am not mistaken) there are a few solutions. Two of them are:
- Create all
Hilbert
instances directly from JSON in the test code, not viaHamiltonian
, or - Decouple the instantiation of
Hilbert
fromHamiltonian
and do not have the later own the Hilbert object (this is, I think, somewhat natural: The Hilbert space exists independently of the Hamiltonian of the system, so why have theHamiltonian
class own it). Instead, store a reference (or non-owning pointer) toHilbert
inHamiltonian
. In this case, theHilbert
reference needs to be passed toHamiltonian
in the constructor and its lifetime should be managed in a higher scope in the same place where that ofHamiltonian
. (Note thatGraph
is already passed to theHamiltonian
constructor as reference in the same way.)
I prefer the second solution, but this would require changing the code in several places to always create Hilbert
outside of Hamiltonian
. What do you think?
(Alternatively, it is also possible to rewrite the test code to deal with both free and Hamiltonian-owned Hilbert
objects, but I think this would be more workaround than solution.)