Fix factor of 2 on gradients of models with real parameters

Created by: PhilipVinc

I think the gradient for real-valued models we are using is wrong

I think we forgot to account for a factor of 2 that arises from taking the real part when parameters are real. When weights are complex, I think the gradient is correct.

Proving that this gradient was wrong, you can see that in the test we were dividing by two the finite-difference gradient for no reason when weights were real.

(Noticed because when doing the dynamics of some model with real parameter my timescale was off by a factor of 2 wrt the exact dynamics, but I'll double check...)

THIS IS BREAKING because some models will now have a gradient doubled (so learning rate must be changed) but I think it's a bug fix

Created by: github-actions[bot]

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/fix-real-grad

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

Created by: gcarleo

Formatting

Created by: gcarleo

Definitely a bug fix, doesn't matter if it's breaking, it's breaking for good reasons!

Created by: PhilipVinc

Yeah, I agree. I'd like to triple-check however that this is correct. I'll try rerunning the dynamics... Also @femtobit might want to give a look.

Created by: gcarleo

just to make sure I understand, is this the factor of 2 in Eq. 3.15 ? https://gitlab.com/nqs/ictp_school/-/blob/master/notes.pdf

Created by: femtobit

Yes, I haven't used real-parameter models with the dynamics code here, so I can imagine this bug remaining unnoticed so far. And as far as I recall, NetKet 2 did have the factor of two.

We should maybe also add a real-parameter dynamics example (like Ising with mod-phase RBM) to cover this case.

Created by: PhilipVinc

@gcarleo yes it is. And it only appears for real parameters

Created by: gcarleo

yes then we all agree this is a bugfix

Created by: gcarleo

I am just wondering what happens if you have mixed real and complex values

Created by: PhilipVinc

You'll get the factor of two on the real ones and no factor on the complex ones, which is consistent with the equation you wrote

Created by: gcarleo

yes but I guess for consistency as soon as you mixed types it's easier to treat everything as real (thus separating real and imaginary part for the compelx parameters)

Created by: PhilipVinc

what do you mean exactly? I'm not sure I understand what you mean.... We rely on (nk)jax.vjp which always splits real and imaginary parts internally, but then recomposes them when it returns the <Ok*E_loc> term. However that's an implementation detail of how the vip product is done.

We then need to take the real part according to Eq. 3.15 if those parameters are real (and not take it if parameters are complex).

How could we do this differently?

Created by: gcarleo

I am thinking about how this would play along with QGT when you have mixed types for the parameters, maybe this is already OK, but just curious to know

Created by: gcarleo

the main problem is holomorphicity

Created by: PhilipVinc

(Ah. They already work out correctly. For QGTOnTheFly that's because we use nkjax.vjp which splits real and imaginary parts and internally treats everything as real. QGTJacobianPyTree splits everything as well, and so it's good, if holmorphic=False/unspecified (I think it also splits internally for holomorphic=True) QGTJacobianDense does not support mixed types for parameters so it does not matter.

closed

reopened

Created by: PhilipVinc

the main problem is holomorphicity

Things like expect_and_grad and QGTOnTheFly which are written only using vjp and jvp products, don't care about holomorphicity because vip/jvp split everything and therefore don't care about it.

Created by: PhilipVinc

All relevant tests pass, and nothing else breaks!

The only missing non-passing test is test/variational/test_continuous_expect.py:test_expect which is testing against some hard-coded values... @gpescia why did you put that factor of 2 in front of the gradient? Was it to make the test pass?

Created by: gcarleo

Review: Approved

good to go, just fix the formatting!