Moving extract_atom types functoin from reader/xps to nexusutils/help… #94
Conversation
a32c7f4 to
905838e
Compare
Pull Request Test Coverage Report for Build 4765763654
💛 - Coveralls |
905838e to
5e22ed7
Compare
5e22ed7 to
e268c90
Compare
|
Handling of atoms does work which is logically unrelated to parsing so instead of placing these functions in the generic helpers.py better would be to make py file with a speaking name were all this atom handling is stored this file can be in the same place like helpers at the same level of the directory tree in this way one could then import it selectively where needed, I agree that such functions should be in the generic place for the dataconverter as it is indeed cross-reader |
| atom_types: set = set() | ||
| element: str = "" | ||
| # tested with "(C38H54S4)n(NaO2)5(CH4)NH3B" | ||
| for char in formula: |
There was a problem hiding this comment.
How does it compare with the solution in NOMAD? Any reason why we not take the same solution as the one there (https://gitlab.mpcdf.mpg.de/nomad-lab/nomad-FAIR/-/blob/develop/nomad/parsing/nexus.py ase lib used below line # 320)?
There was a problem hiding this comment.
Yes, we could use the the example their with pymatgen. The p 8000 oint is only to extract the chemical formula if we use pymatgen nexusutils will be havier. I think this is not a good practice. Frankly, I tried to use 'ase' lib or is their any function to do it, as we already have 'ase' in dependencies list.
There was a problem hiding this comment.
In fact lines 343-345 are doing the job there. The question if this code should do the same or not? And also what Markus asked if and how 'D' is handled, etc.
|
|
||
| test_chemical_formula = "(C38H54S4)n(NaO2)5(CH4)NH3B" | ||
| expected_atom_types = ['C', 'H', 'B', 'N', 'Na', 'O', 'S'] | ||
|
|
There was a problem hiding this comment.
if we have difference between our solution here and the ase based solution in NOMAD, please also add an example which shows how it should work where the another solution fails on!
| def extract_atom_types(formula: str, mode: str = 'hill') -> Union[Set, List]: | ||
| def convert_to_hill(atoms_typ): | ||
| """Convert list of atom into to hill.""" | ||
| if not isinstance(atoms_typ, list) and isinstance(atoms_typ, set): |
There was a problem hiding this comment.
there should be a check which catches the case when atoms_typ is empty or neither list nor set
There was a problem hiding this comment.
what if atoms_typ has D (deuterium)?
| """ | ||
|
|
||
| test_chemical_formula = "(C38H54S4)n(NaO2)5(CH4)NH3B" | ||
| expected_atom_types = ['C', 'H', 'B', 'N', 'Na', 'O', 'S'] |
There was a problem hiding this comment.
should test with all words available in the Hill vocabulary
7e95749 to
8ceb132
Compare
8ceb132 to
2d7e47f
Compare
#94) * Moving extract_atom types functoin from reader/xps to nexusutils/helper module. * Moving extract_atom types functoin from reader/xps to nexusutils/help * Working on requested changes. * Working on requested changes. --------- Co-authored-by: RubelMozumder <rubelmozumder@outlook.com>
…er module.