search

lsmo-epfl / aiida-lsmo

AiiDA workflows for the LSMO laboratory at EPFL
Other
9 stars 13 forks source link

integrate oxidation state prediction into CP2K workchains #66

Closed ltalirz closed 3 years ago

ltalirz commented 3 years ago

It is now possible to specify the initial_magetization in cp2k workchains in the following ways:

Users can also provide user-defined values in the form of a dictionary element=>magnetization or a dictionary in the form of the initial_magnetizations.yaml file.

ltalirz commented 3 years ago

@danieleongari In addition to the "tagged structuredata" that is provided here, you may also want a dictionary kind -> oxidation state.

I'm happy to make the changes if you let me know where the changes should go -

  1. should there be a switch for using oximachine in the protocol.yaml (I guess yes?)?
  2. in which protocols should it be "on"? should there be a new one?
  3. i suggest the oxidation states will then be computed once in setup_multistage and then passed around to be used in get_input_multiplicity and get_kinds_section
danieleongari commented 3 years ago

Hi, thanks for your work!

  1. Here we need to provide some conversion table: kind_magnetization(element, oxidation_state). I suggest to make it as a .csv file in the aiida_lsmo/workchains/cp2k_multistage_protocols/ so that later we can use different conventions, since we are adding and extra degree of freedom.
  2. Yes, instead of using elemennts, the user needs to specify in protocol.yaml something like: `initial_magnetization: { logic: highest_magnetization }. Let's not complicate stuff: one can or specify all the atoms or use the logic. If the user wants to specify a magetization he specifies all the atoms. LEt's also introduce the logic: if no logic specified and no element specified, that element witll have magnetization 0.
  3. At the moment, I'm using specific yaml protocols loaded as input: we can test this way and later decide how to set defaults.
  4. Ok, I will consider your suggestion when introducing kind_magnetization(element, oxidation_state).

One last concern: we need to be careful about oximachine running locally when hundreds/thousands or jobs are submitted!

ltalirz commented 3 years ago

One last concern: we need to be careful about oximachine running locally when hundreds/thousands or jobs are submitted!

Yes... however, I'm not sure it make sense to move it somewhere else

ltalirz commented 3 years ago

@danieleongari I've incorporated your table into the workchain now.

Unfortunately, there is a bit of logic needed to do the "tagging" + the mapping to magnetization states; in order to avoid having too many function calls and variables passed around I created a class for it.

It's still not very elegant; one can probably still improve it.

ltalirz commented 3 years ago

Ah hm, I see that in examples/test_multistage_aluminum.py the initial_magnetization is actually specified manually (and only for Al). https://github.com/lsmo-epfl/aiida-lsmo/pull/66/checks?check_run_id=1566317182#step:6:296

I can reintroduce this as a possibility... just a bit more logic

danieleongari commented 3 years ago

Thanks, it is now working great. I used this script as example, if you want to adapt it as a test:

import ase

from aiida.plugins import DataFactory, WorkflowFactory
from aiida import cmdline
from aiida import engine
from aiida.orm import Dict, StructureData, Str, Int, SinglefileData, Code

# Workchain objects
Cp2kMultistageWorkChain = WorkflowFactory('lsmo.cp2k_multistage')

# Data objects
StructureData = DataFactory('structure')

# testing user change of parameters and protocol
parameters = Dict(dict={'FORCE_EVAL': {'DFT': {'MGRID': {'CUTOFF': 250,}}}})

# Construct process builder
builder = Cp2kMultistageWorkChain.get_builder()
builder.structure = StructureData(ase=ase.io.read('../tests/data/Cu-I-II-HKUST-1.cif'))
builder.protocol_tag = Str('standard')
builder.cp2k_base.cp2k.parameters = parameters
builder.cp2k_base.cp2k.code = Code.get_from_string('cp2k-5.1@helvetios')
builder.cp2k_base.cp2k.metadata.options.resources = {
    'num_machines': 1,
 #   'num_mpiprocs_per_machine': 32,
}
builder.cp2k_base.cp2k.metadata.options.max_wallclock_seconds = 1 * 60 * 60

wc = engine.submit(builder)
ltalirz commented 3 years ago

the last failing test is because something changed in the cp2k input of the binding energy workchain (and I would need to regenerate the test data). this leads to the general question, for which protocols the oxidation state prediction should be the default

we now have three pre-defined methods: "zero" (no initial mags), "element" (using default oxidation state) and "oxidation_state" (using predicted oxidation state)

codecov-io commented 3 years ago

Codecov Report

Merging #66 (5dbd409) into develop (5ceff5d) will increase coverage by 0.28%. The diff coverage is 88.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #66      +/-   ##
===========================================
+ Coverage    77.67%   77.96%   +0.28%     
===========================================
  Files           31       31              
  Lines         2271     2355      +84     
===========================================
+ Hits          1764     1836      +72     
- Misses         507      519      +12
Impacted Files Coverage Δ
aiida_lsmo/workchains/cp2k_multistage.py 87.84% <79.16%> (-1.25%) :arrow_down:
aiida_lsmo/workchains/cp2k_binding_energy.py 84.00% <83.33%> (+0.12%) :arrow_up:
aiida_lsmo/utils/cp2k_utils.py 84.31% <88.00%> (-8.19%) :arrow_down:
...o/workchains/cp2k_multistage_protocols/__init__.py 92.40% <92.53%> (-2.60%) :arrow_down:
aiida_lsmo/calcfunctions/oxidation_state.py 100.00% <100.00%> (+16.66%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5ceff5d...5dbd409. Read the comment docs.

ltalirz commented 3 years ago

Note: In the last CI run only the coverage upload failed - tests passed.