Skip to content

WIP: Replace shared_ptr with unique_ptr in Hamiltonian and Hilbert

Vicentini Filippo requested to merge github/fork/femtobit/unique-ptr into master

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 via Hamiltonian, or
  • Decouple the instantiation of Hilbert from Hamiltonian 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 the Hamiltonian class own it). Instead, store a reference (or non-owning pointer) to Hilbert in Hamiltonian. In this case, the Hilbert reference needs to be passed to Hamiltonian in the constructor and its lifetime should be managed in a higher scope in the same place where that of Hamiltonian. (Note that Graph is already passed to the Hamiltonian 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.)

Merge request reports