MRChemSoft / mrchem

MultiResolution Chemistry
GNU Lesser General Public License v3.0
29 stars 21 forks source link

Can we make `QMPotential` not virtual? #330

Closed ilfreddy closed 4 years ago

ilfreddy commented 4 years ago

@stigrj and @robertodr

I think QMPotential should be made real (non virtual).

  1. Currently only setup() and clear() are preventing QMPotential from being a non-virtual class. In several cases those two routines reduce however to void setup(double prec) override { setApplyPrec(prec); } and void clear() override { clearApplyPrec(); }.
  2. It is meaningful to consider a generic potenital for a particle, which does not need more than just the definition of the potential function.

I would then suggest to use the definintions of those two methods as the generic ones and overridde those only when needed.

stigrj commented 4 years ago

agreed

ilfreddy commented 4 years ago

I will prepare a PR

ilfreddy commented 4 years ago

Question: why is free(NUMBER::Total) in the destructor for NuclearPotential but it is in the clear() method for all other classes? Is there a reason for it?

stigrj commented 4 years ago

Depends on whether the potential is built in the constructor or in setup. Originally, all potentials were built with precision prec in setup(prec) and deleted in clear(), which is the reason these were made virtual in the first place. But then the NuclearPotential was so time consuming to build, so it was better to move it's projection to the constructor.

This makes a difference when we're using dynamic precision in the SCF, starting with low start_prec and ending with high final_prec. So instead of re-building the NuclearPotential with current_prec in every iteration, it is instead build with final_prec in the constructor, and then we only vary the apply_prec through setup.

So whether the potential should be build in the constructor or in setup will depend on the operator, e.g. the CoulombPotential must be built in setup because it depends on the current orbitals, but if the operator is "static" it should probably be built in the constructor (note that also memory footprint needs to be considered here).

ilfreddy commented 4 years ago

Thanks. Then I will leave NuclearPotential as it was.