MRChemSoft / mrchem

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

Lack of radii in Mantina set should raise a warning, not an error #476

Closed Gabrielgerez closed 6 months ago

Gabrielgerez commented 8 months ago

This line of code stops the program from running if the radius of any species is not in the mantina set regardless if we are running solvent or not https://github.com/MRChemSoft/mrchem/blame/9de2280c94bf3cd2576778d4c79a7cc3a3d1e5bd/python/mrchem/validators.py#L355

I think this should not stop the execution of the parser, but actually raise a warning such that the advanced user can fix the radii after the parser is finished. In addition, it should not stop execution of the code if one is not running solvent, which it does now.

What is your opinion on this @robertodr ?

robertodr commented 8 months ago

I think this should not stop the execution of the parser, but actually raise a warning such that the advanced user can fix the radii after the parser is finished.

I don't agree. I prefer to be conservative and stop (very) early if there's a possibility of running a meaningless calculation. Raising the warning would not be enough of a signal in this case.

In addition, it should not stop execution of the code if one is not running solvent, which it does now.

Here I agree. Can you give an example of non-solvent input that crashes because of this? A test should be added for it. The problem is that self.validate_cavity is called no matter what, while it only makes sense to call it if we're doing a solvent calculation...

Gabrielgerez commented 8 months ago

I think this should not stop the execution of the parser, but actually raise a warning such that the advanced user can fix the radii after the parser is finished.

I don't agree. I prefer to be conservative and stop (very) early if there's a possibility of running a meaningless calculation. Raising the warning would not be enough of a signal in this case.

I can agree on this, but I find the error message slightly useless.

ValueError: vdw-radius for ru not defined in the Mantina set.

Maybe we should add this

ValueError: vdw-radius for ru (index 0) not defined in the Mantina set. Either explicitly add radius or edit python/template.yml and update the parser

In addition, it should not stop execution of the code if one is not running solvent, which it does now.

Here I agree. Can you give an example of non-solvent input that crashes because of this? A test should be added for it. The problem is that self.validate_cavity is called no matter what, while it only makes sense to call it if we're doing a solvent calculation...

Here is an example which will stop execution:

# vim:syntax=sh:

world_prec = 1.0e-3               # Overall relative precision
world_size = 5                    # Size of simulation box 2^n

MPI {
  numerically_exact = true        # Guarantee identical results in MPI
}

Molecule {
$coords
Ru   0.0     0.0    0.0
$end
}

WaveFunction {
  method = HF                     # Wave function method (HF or DFT)
}

SCF {
  kain = 3                        # Length of KAIN iterative history
  max_iter = 5
  orbital_thrs = 1.0e-2           # Convergence threshold in orbital residual
  guess_type = SAD_DZ             # Type of initial guess: none, mw, gto
}