Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically set the relative atomic mass according to "type_map" #867

Merged
merged 14 commits into from
Aug 16, 2022
Merged

Automatically set the relative atomic mass according to "type_map" #867

merged 14 commits into from
Aug 16, 2022

Conversation

panxiang126
Copy link
Contributor

A new function get_atomic_masses was written to get the relative atomic mass (data from: Atomic weights of the elements 2013 (IUPAC Technical Report)). So that there is no need to specify the mass_map manually in param.json.

Automatically set the relative atomic mass according to "type_map"

Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Automatically set the relative atomic mass according to "type_map"

Signed-off-by: Pan Xiang <panxiang126@gmail.com>
@panxiang126 panxiang126 changed the title Automatically set the relative atomic mass according to "type_map"Devel Automatically set the relative atomic mass according to "type_map" Aug 12, 2022
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
def get_atomic_masses(chemical_symbol):
chemical_symbols = ['H', 'He', 'Li', 'Be', 'B', 'C', 'N', 'O', 'F', 'Ne', 'Na', 'Mg', 'Al', 'Si', 'P', 'S', 'Cl', 'Ar', 'K', 'Ca', 'Sc', 'Ti', 'V', 'Cr', 'Mn', 'Fe', 'Co', 'Ni', 'Cu', 'Zn', 'Ga', 'Ge', 'As', 'Se', 'Br', 'Kr', 'Rb', 'Sr', 'Y', 'Zr', 'Nb', 'Mo', 'Tc', 'Ru', 'Rh', 'Pd', 'Ag', 'Cd', 'In', 'Sn', 'Sb', 'Te', 'I', 'Xe', 'Cs', 'Ba', 'La', 'Ce', 'Pr', 'Nd', 'Pm', 'Sm', 'Eu', 'Gd', 'Tb', 'Dy', 'Ho', 'Er', 'Tm', 'Yb', 'Lu', 'Hf', 'Ta', 'W', 'Re', 'Os', 'Ir', 'Pt', 'Au', 'Hg', 'Tl', 'Pb', 'Bi', 'Po', 'At', 'Rn', 'Fr', 'Ra', 'Ac', 'Th', 'Pa', 'U', 'Np', 'Pu', 'Am', 'Cm', 'Bk', 'Cf', 'Es', 'Fm', 'Md', 'No', 'Lr', 'Rf', 'Db', 'Sg', 'Bh', 'Hs', 'Mt', 'Ds', 'Rg', 'Cn', 'Nh', 'Fl', 'Mc', 'Lv', 'Ts', 'Og']

# doi:10.1515/pac-2015-0305
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the latest data?
https://doi.org/10.1515/pac-2019-0603

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not fully understand wanghan-iapcm's comment below. I have rewritten the get_atomic_masses function, and now it can be called by three types, i. e. element_names, chemical_symbols and atomic_number. I am not sure if this will remove his/her concern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean atom name can be ANY string, not necessary element_names or chemical_symbols.
For example it is valid to write "type_map" : ["foo", "bar"].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @wanghan-iapcm. This should be documented that auto only supports the standard atoms. (For example, OW will not be supported)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the documentation.

@@ -1115,7 +1115,7 @@ def _make_model_devi_native(iter_index, jdata, mdata, conf_systems):
model_devi_taup = 0.5
if 'model_devi_taup' in jdata :
model_devi_taup = jdata['model_devi_taup']
mass_map = jdata['mass_map']
mass_map = [get_atomic_masses(i) for i in jdata['type_map']]
Copy link
Member

@njzjz njzjz Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not override the user config, as one may want to simulate isotopes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Please check if jdata['mass_map'] is None is used correctly

Copy link
Contributor

@wanghan-iapcm wanghan-iapcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dpgen and dpdata do not restrict the atom_names to chemical symbols. This PR will break the convention.

Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #867 (557215b) into devel (ed86867) will decrease coverage by 0.00%.
The diff coverage is 36.00%.

@@            Coverage Diff             @@
##            devel     #867      +/-   ##
==========================================
- Coverage   38.00%   37.99%   -0.01%     
==========================================
  Files          99       99              
  Lines       17754    17775      +21     
==========================================
+ Hits         6747     6754       +7     
- Misses      11007    11021      +14     
Impacted Files Coverage Δ
dpgen/generator/run.py 62.21% <33.33%> (-0.30%) ⬇️
dpgen/tools/relabel.py 14.73% <33.33%> (ø)
dpgen/generator/arginfo.py 99.27% <100.00%> (+0.01%) ⬆️
dpgen/simplify/arginfo.py 96.55% <0.00%> (+0.25%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@njzjz njzjz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the following documentation to reflect your changes.

doc_mass_map = 'Standard atom weights.'

@@ -3733,6 +3734,34 @@ def run_iter (param_file, machine_file) :
raise RuntimeError ("unknown task %d, something wrong" % jj)
record_iter (record, ii, jj)

def get_atomic_masses(atom):

element_names = ['Hydrogen', 'Helium', 'Lithium', 'Beryllium', 'Boron', 'Carbon', 'Nitrogen', 'Oxygen', 'Fluorine', 'Neon', 'Sodium', 'Magnesium', 'Aluminium', 'Silicon', 'Phosphorus', 'Sulfur', 'Chlorine', 'Argon', 'Potassium', 'Calcium', 'Scandium', 'Titanium', 'Vanadium', 'Chromium', 'Manganese', 'Iron', 'Cobalt', 'Nickel', 'Copper', 'Zinc', 'Gallium', 'Germanium', 'Arsenic', 'Selenium', 'Bromine', 'Krypton', 'Rubidium', 'Strontium', 'Yttrium', 'Zirconium', 'Niobium', 'Molybdenum', 'Technetium', 'Ruthenium', 'Rhodium', 'Palladium', 'Silver', 'Cadmium', 'Indium', 'Tin', 'Antimony', 'Tellurium', 'Iodine', 'Xenon', 'Caesium', 'Barium', 'Lanthanum', 'Cerium', 'Praseodymium', 'Neodymium', 'Promethium', 'Samarium', 'Europium', 'Gadolinium', 'Terbium', 'Dysprosium', 'Holmium', 'Erbium', 'Thulium', 'Ytterbium', 'Lutetium', 'Hafnium', 'Tantalum', 'Tungsten', 'Rhenium', 'Osmium', 'Iridium', 'Platinum', 'Gold', 'Mercury', 'Thallium', 'Lead', 'Bismuth', 'Polonium', 'Astatine', 'Radon', 'Francium', 'Radium', 'Actinium', 'Thorium', 'Protactinium', 'Uranium', 'Neptunium', 'Plutonium', 'Americium', 'Curium', 'Berkelium', 'Californium', 'Einsteinium', 'Fermium', 'Mendelevium', 'Nobelium', 'Lawrencium', 'Rutherfordium', 'Dubnium', 'Seaborgium', 'Bohrium', 'Hassium', 'Meitnerium', 'Darmastadtium', 'Roentgenium', 'Copernicium', 'Nihonium', 'Flerovium', 'Moscovium', 'Livermorium', 'Tennessine', 'Oganesson']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use 4 spaces for indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
@panxiang126 panxiang126 requested a review from njzjz August 15, 2022 02:47
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
doc_use_ele_temp = 'Currently only support fp_style vasp. \n\n\
- 0: no electron temperature. \n\n\
- 1: eletron temperature as frame parameter. \n\n\
- 2: electron temperature as atom parameter.'

return [
Argument("type_map", list, optional=False, doc=doc_type_map),
Argument("mass_map", list, optional=False, doc=doc_mass_map),
Argument("mass_map", [str, list], optional=True, default="auto", doc=doc_mass_map),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: as the arguments are still being built, at present the default value will not be automatically applied, but only shown in the documentation.

Signed-off-by: Pan Xiang <panxiang126@gmail.com>
@panxiang126 panxiang126 requested a review from njzjz August 15, 2022 07:37
@panxiang126 panxiang126 requested a review from njzjz August 16, 2022 06:44
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Signed-off-by: Pan Xiang <panxiang126@gmail.com>
@AnguseZhang AnguseZhang merged commit 544953e into deepmodeling:devel Aug 16, 2022
@njzjz
Copy link
Member

njzjz commented Aug 16, 2022

@AnguseZhang you should squash merge...

@@ -968,7 +968,7 @@ def _make_model_devi_revmat(iter_index, jdata, mdata, conf_systems):
sys_idx = expand_idx(cur_job['sys_idx'])
if (len(sys_idx) != len(list(set(sys_idx)))) :
raise RuntimeError("system index should be uniq")
mass_map = jdata['mass_map']
mass_map = [get_atomic_masses(i) for i in jdata['type_map']] if jdata['mass_map'] == "auto" else jdata['mass_map']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to write a function for automatic building of mass_map rather than repeating the same line for multiple times.

@wanghan-iapcm
Copy link
Contributor

@AnguseZhang I am against the merging of this PR.

@AnguseZhang
Copy link
Collaborator

@AnguseZhang I am against the merging of this PR.

@wanghan-iapcm Reverted in #881

wanghan-iapcm pushed a commit that referenced this pull request Aug 27, 2022
For previous discussions, see
[#867](#867) and
[#881](#881).

It is very strange that the comparison is with the version before
[#881](#881).

Signed-off-by: Pan Xiang <panxiang126@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants