search

muon-spectroscopy-computational-project / pymuon-suite

Collection of scripts and utilities for muon spectroscopy
GNU General Public License v3.0
8 stars 7 forks source link

pm-muairss default parameter values set to none in generated struct.yaml files #16

Closed sarahbyrnie closed 3 years ago

sarahbyrnie commented 3 years ago

If unset in the pm-muairss yaml file, the default values for force tolerance and geometry optimization steps get to none in the generated struct.yaml files. This causes an error when running pm-uep-opt

stur86 commented 3 years ago

Right, I think this should be fixable with a single change in pymuonsuite/io/uep.py, specifically in _create_calculator:

        calc.opt_tol = params['geom_force_tol'] or calc.opt_tol

should make it so that if params['geom_force_tol'] is anything fals-y (like None) we just fall back to the default. Is there anything else I'm missing, @lauramurgatroyd ?

lauramurgatroyd commented 3 years ago

Yeah I need to make sure this happens for the other parameters (such as geom steps) as well, it's a problem I introduced when I was changing how the defaults are set. I'll fix this today.

lauramurgatroyd commented 3 years ago

I am not actually sure what the correct default value should be for the opt_tol in the UEP method.

In pymuonsuite before I did the restructuring, if the opt_tol had not been set in the yaml file then the schema automatically set it to 0.05 : https://github.com/muon-spectroscopy-computational-project/pymuon-suite/blob/2bb940dd1e96e068d6968a9244237630c6e05e11/pymuonsuite/schemas.py#L171

In the constructor for the UEP calculator, the value is 1e-5: https://github.com/muon-spectroscopy-computational-project/pymuon-suite/blob/2bb940dd1e96e068d6968a9244237630c6e05e11/pymuonsuite/io/uep.py#L37

But in muairss, the UEP calculator was created and then the value of opt_tol immediately overwritten by the default value from the yaml file or default from the schema: https://github.com/muon-spectroscopy-computational-project/pymuon-suite/blob/2bb940dd1e96e068d6968a9244237630c6e05e11/pymuonsuite/muairss.py#L224

This meant that the overall effect was to set the default as 0.05. Is this what should be happening? If so, why is the default value set to 1e-5 in the constructor of the UEP calculator? Should we change that?

(Note here I have linked to old versions of the code from before my restructuring pull request, just to demonstrate how it used to work)

lauramurgatroyd commented 3 years ago

I just went and checked the other calculators too, to make sure the defaults were as expected.

Before the restructuring of read/write, for castep, the schema was setting the default for the max_scf_steps to 200, but then here: https://github.com/muon-spectroscopy-computational-project/pymuon-suite/blob/2bb940dd1e96e068d6968a9244237630c6e05e11/pymuonsuite/muairss.py#L163 it was trying to fall back and set it as 30, even though it had already automatically been set to 200 in the schema.

Which do we want to be the default?

I can see that in CASTEP the default value is 30. Should we be consistent with that? Or do we want the default scf cycles to be the same (at 200) for all methods in pymuonsuite? https://www.tcm.phy.cam.ac.uk/castep/documentation/WebHelp/content/modules/castep/keywords/k_max_scf_cycles_castep.htm

stur86 commented 3 years ago

@lauramurgatroyd the reason why the default for the UEP is much finer is that in UEP it's easily possible to achieve that level of precision, while with DFT it would be endless pain and grinding of teeth to do the same. So it's a much more reasonable default for UEP. Ideally, I think we would want to make it so that if it hasn't been specified by the user in the global .yaml file, then it doesn't propagate in the generated ones either.