NLESC-JCER / QMCTorch

Pytorch Implementation of Real Space Quantum Monte Carlo Simulations of Molecular Systems
https://qmctorch.readthedocs.io/
Apache License 2.0
23 stars 2 forks source link

Review JOSS #145

Closed scemama closed 7 months ago

scemama commented 1 year ago

Hi!

Here are a few comments to help improve the paper (https://github.com/openjournals/joss-reviews/issues/5472):

scemama commented 1 year ago
AbdAmmar commented 1 year ago

Hello ! In addition to Anthony's comments,

It would be helpful to include further references in Line 25

and in the "Backflow Transformation" section, Line 52

NicoRenaud commented 10 months ago

@scemama Thanks a lot for all the corrections, I've implemented all of them in #148 For the determinants I've used $\sum_n c_n D_n^\uparrow Dn^\downarrow$ as it is what I have implemented in the code. I could implement $\sum{ij} c_{ij} D_i^\uparrow D_j^\downarrow$. Would you recommend that ?

@AbdAmmar Thanks a lot for all the references. I've added them all. Computing the Laplacian of the Slaters with automatic differentiation is extremely slow as I would have to compute the full Hessian and extract the diagonal. I can make a small benchmark of that approach compared to the Jacobi trace I'm using by default. Automatic differentiation is used to compute all the derivative wrt the wave function parameters. I don't believe that this is faster than using analytic expression, but you don't have to derive those expression. I will make a small benchmark of the computational performance.

Thanks again to both of you for your input and apologies for the time it took me to work on it Best

Nico

scemama commented 10 months ago

@scemama Thanks a lot for all the corrections, I've implemented all of them in #148 For the determinants I've used

as it is what I have implemented in the code. I could implement

. Would you recommend that ?

Yes, if you have large CI expansions! You would only need to compute the unique set of $D_i^\uparrow$ and $Dj^\downarrow$ and their gradients and Laplacians. Then you can recombine everything to get the total wave function. This his how we obtain in QMC=Chem a $\mathcal{O}(\sqrt{N{det}})$ scaling for near-FCI wave functions. You can have a look at this presentation: http://irpf90.ups-tlse.fr/files/pacifichem.pdf or this paper: https://arxiv.org/abs/1510.00730 we can also have a zoom to if you want more detail :-)

tonnylou44853 commented 8 months ago

Hi!

In addition to the comments above, here is a list of typos I found in the docs:

NicoRenaud commented 8 months ago

@scemama Thanks a lot for all the corrections, I've implemented all of them in #148 For the determinants I've used as it is what I have implemented in the code. I could implement . Would you recommend that ?

Yes, if you have large CI expansions! You would only need to compute the unique set of Di↑ and Dj↓ and their gradients and Laplacians. Then you can recombine everything to get the total wave function. This his how we obtain in QMC=Chem a O(Ndet) scaling for near-FCI wave functions. You can have a look at this presentation: http://irpf90.ups-tlse.fr/files/pacifichem.pdf or this paper: https://arxiv.org/abs/1510.00730 we can also have a zoom to if you want more detail :-)

Thanks for the explanation @scemama ! That sounds like something I will have to do indeed :) I won't have too much time to develop new features on qmctorch before the end of the year. But I will pick this up in January ! I will come back to you ! Thanks !

NicoRenaud commented 8 months ago

Hi!

In addition to the comments above, here is a list of typos I found in the docs:

Thanks @tonnylou44853 ! I've made all the change and will merge as soon as the test are ok ! :) Thanks for all the input

NicoRenaud commented 7 months ago

Thanks a lot everyone for all the input !! I'm closing that issue now :)