search

lsmo-epfl / aiida-lsmo

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

Initial magnetization: complete 3rd and 4th raw of the periodic table #37

Closed yakutovicha closed 4 years ago

codecov-commenter commented 4 years ago

Codecov Report

Merging #37 into develop will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #37   +/-   ##
========================================
  Coverage    36.31%   36.31%           
========================================
  Files           22       22           
  Lines         1586     1586           
========================================
  Hits           576      576           
  Misses        1010     1010

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 af5e894...eaaab28. Read the comment docs.

danieleongari commented 4 years ago

This PR adds oxidation states for more elements, allowing MultistageWorkChain to handle more elements (the work chain will fail if no oxidation is specified). However, this makes emerge one problem of these "default" protocols called with protocol_tag: they are not stored by AiiDA but simply loaded, and changing them can be difficult to track see: https://github.com/lsmo-epfl/aiida-lsmo/blob/af5e894e024e38bde5349184f482fdd264b21c37/aiida_lsmo/workchains/cp2k_multistage.py#L198-L200

In this case, moreover, oxidation states are not obvious, and what I have been doing is to keep my personal extended list in my protocol.yaml for testing purpose, before merging. In my list, I came up with different reference oxidation states:

 Ga: 0  # oxidation state +3
  Ge: 0  # nonmetal  
  As: 0  # nonmetal
  Se: 0  # nonmetal
  Br: 0  # nonmetal
  Kr: 0  # nonmetal
  Rb: 0  # oxidation state +1
  Sr: 0  # oxidation state +2
  Y:  0  # oxidation state +3
  Zr: 0  # oxidation state +4
  Nb: 1  # oxidation state +4
  Mo: 2  # oxidation state +4
  Tc: 5  # oxidation state +2
  Ru: 4  # oxidation state +2
  Rh: 3  # oxidation state +2
  Pd: 2  # oxidation state +2
  Ag: 0  # oxidation state +1
  Cd: 0  # oxidation state +2
  In: 0  # oxidation state +3
  Sn: 2  # oxidation state +2
  Sb: 0  # nonmetal
  Te: 0  # nonmetal
  I:  0  # nonmetal  
  Xe: 0  # nonmetal

Not to end up with different version of the standard protocol I would suggest to load the protocol form a SinglefileData YAML with your values instead of proceeding with this PR.

The long term suggestion I have to make all the coding more elegant is to keep the current standard, test, ... YAML files in cp2k_multistage_protocol but having a function that converts them to Dict objects and make them available in the plugin, so that someone can pass them to AiiDA in a way such as:

from aiida_lsmo.cp2k_protocols import standard_protocol
builder.protocol_yaml = standard_protocol

(maybe changing the protocol_yaml name to something else) so that the protocol gets stored as a Dict object instead of just a Str (e.g., standard).

What do you think? No need to do this modifications now, let's open an issue, and you can continue working with your protocol as SinglefileData YAML. If you find more useful to merge the PR it's ok for me, but these problems I raised will make your work difficult to reproduce later.