Skip to content
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

Add delayed update #242

Merged
merged 33 commits into from
Jun 12, 2019
Merged

Add delayed update #242

merged 33 commits into from
Jun 12, 2019

Conversation

ye-luo
Copy link
Collaborator

@ye-luo ye-luo commented Jun 6, 2019

Closes #239.
Unit test and "check_wfc -f Det" are added.

@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 6, 2019

test this please

@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 6, 2019

test this please

PDoakORNL and others added 3 commits June 7, 2019 15:52
Absolute error checking is quite unreliable on deteriminants.
MP build is not checked with deteriminant.
@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 7, 2019

Test this please

@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 10, 2019

Test this please

Copy link
Collaborator

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvements to the build and check_det is gone also good. But import of ParticleSet, EinsplineSPO and DiracDeterminant entanglements not previously an issue in miniqmc. Whether those should be added to develop is a question of what miniqmc is supposed to be...

src/QMCWaveFunctions/DiracDeterminantRef.h Show resolved Hide resolved
src/QMCWaveFunctions/DiracMatrix.h Outdated Show resolved Hide resolved
src/QMCWaveFunctions/DiracMatrix.h Outdated Show resolved Hide resolved
src/QMCWaveFunctions/einspline_spo.hpp Show resolved Hide resolved
src/Utilities/scalar_traits.h Outdated Show resolved Hide resolved
@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 12, 2019

I think the added entanglements are necessary for real QMC algorithm and QMCPACK needs.

Ownership of SPO.
When the trial wavefunction is Slater-Jastrow type, it may contains determinants using SPOs. When the whole wavefunction doesn't contain Slater determinant, then there is no SPO. Then SPO needed to be owned by determinants. The code is more modular and composable.
If SPO is owned outside determinants, we have to deal with a null pointer or size 0 vector to handle SPO. This is not a clean route and indicates a non-generic design.

EinsplineSPO or base class SPOSet.
Even though that the miniQMC only supports spline SPO, the design needs to be flexible to be incorporated back to QMCPACK. So hard coding EinsplineSPO and fulfilling only EinsplineSPO needs should be avoided. I'm not saying SPOSet is perfect and believe some call interfaces needs to be updated.

SPOSet using ParticleSet instead of particle positions.
SPOSet represents all the SPOs including LCAO and hybrid representation. Those calculation needs electron ion-distances. ParticleSet owns the particle position and distance tables. Whether to use or not all the data owned by ParticleSet depends on the derived SPO class. I would argue that ParticleSet is better than position only because ParticleSet is more generic.
SPOSet::evaluate(ParticleSet elecs, int iel) is more generic than diverged EinsplineSPO::evaluate(Pos_t R) and LCAO::evaluate(Pos_t R, Pos_v ions) or LCAO::evaluate(Pos_t R, Pos_v displ_ions)
Passing ParticleSet should not stop you doing anything achievable with positions only.

@PDoakORNL
Copy link
Collaborator

I don't understand the argument about the harmonic oscillator basis, but if you are just going to appeal to the main source code everytime miniqmc simply isn't useful for design discussions.

I think it's much clearer if the actual terms needed to evaluate the function are in the function signature. If that requires specialized code that is fine there is an important detail that at least the einspline SPO doesn't have.

So to me the second is much better. The idea that elecs would somehow change the ion positions used to evaluate the LCAO is counter intuitive to me.

@PDoakORNL
Copy link
Collaborator

Anyway make the other changes and I will approve, the ParticleSet discussion is too big for this PR.

@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 12, 2019

I made a mistake harmonic oscillator is a SPO not a WaveFunctionComponent. I correct all the words and removed harmonic oscillator as example.

I would consider the old design as one of the candidates of the new design not being excluded.

I think for virtual functions and functions of templates classes, it is better to have call APIs as generic as possible. If call APIs and classes both need specialization, we cannot solve an all-to-all problem.

@PDoakORNL
Copy link
Collaborator

Seems more like a specialization on an isDependentOnIonPositions trait.

@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 12, 2019

@PDoakORNL Could you correct my following broken code using isDependentOnIonPositions trait?
The following code doesn't compile because EinsplineSPO and LCAO only have one of the two evaluate interface.
Please modify and make it working.

Deteriminant<class SPOType>
{
void ratioGrad()
{
   SPOType spo;
   if(isDependentOnIonPositions<SPOType>::value)
      spo.evaluate(Pos_t R, Pos_v ions); // LCAO only has this interface
   else
      spo.evaluate(Pos_t R); // EinsplineSPO only has this interface
}
}

@PDoakORNL
Copy link
Collaborator

PDoakORNL commented Jun 12, 2019

#include<vector>
#include<type_traits>
#include<iostream>

class LCAOSet
{
 public:
    void evaluate(double pos, std::vector<double>& ions) { std::cout << "ions matter\n"; }
};

class EinsplineSet
{
 public:
    void evaluate(double pos) { std::cout << "what's an ion?\n"; }
};

template <typename t>
struct isDependentOnIonPositions : public std::false_type {};

template<>
struct isDependentOnIonPositions<LCAOSet> : public std::true_type {};

template<class SPOT>
struct StateIShouldNotHave;

template<>
struct StateIShouldNotHave<EinsplineSet>
{
    double pos = 1.0;
};

template<>
struct StateIShouldNotHave<LCAOSet>
{
  double pos = 1.0;
  std::vector<double> ions = {1.0, 2.0, 3.0};
};

template<class SPOT>
class Determinant
{
  SPOT spo;
  StateIShouldNotHave<SPOT> state;
 public:
    template<typename U = SPOT,
       typename std::enable_if<isDependentOnIonPositions<U>::value, std::nullptr_t>::type = nullptr>
    void ratioGrad()
    {
      spo.evaluate(state.pos , state.ions);
    }

    template<typename U = SPOT,
       typename std::enable_if<!isDependentOnIonPositions<U>::value, std::nullptr_t>::type = nullptr>
    void ratioGrad()

    {
      spo.evaluate(state.pos);
    }
};


int main()
{
  Determinant<LCAOSet> det_lcao;
  Determinant<EinsplineSet> det_einsplineset;

  det_einsplineset.ratioGrad();
  det_lcao.ratioGrad();
}

Even xlc++ can do it

Edit: I'd rather the enable_if in the parameter list

@PDoakORNL PDoakORNL merged commit 3b26489 into QMCPACK:develop Jun 12, 2019
@ye-luo
Copy link
Collaborator Author

ye-luo commented Jun 13, 2019

@PDoakORNL Thank you. I expected this complexity. If I have not just one call inside ratioGrad but very long function body, having it repeated the whole function because of 1 call difference, I feel, is not really worth it. If we have a third SPOSet which need another interface, the ratioGrad need a third version. Hopefully you can consider evaluate(const ParticleSet&) and we don't need to maintain this complexity.

The code pattern you showed is very powerful to handle data types or functions from libraries which is often not allowed to be modified. Having traits and SFINAE works very well.

@ye-luo ye-luo deleted the add-delayed-update branch July 31, 2019 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port delayed update algorithms
2 participants