initial investigation into harmonizing particles and atoms #150
Conversation
Pull Request Test Coverage Report for Build 12252256172Details
💛 - Coveralls |
|
|
||
| def __init__(self, m_def: 'Section' = None, m_context: 'Context' = None, **kwargs): | ||
| super().__init__(m_def, m_context, **kwargs) | ||
| self.labels = None |
There was a problem hiding this comment.
This is one approach to simplifying the logic in functions like the composition formula ones, but it seems maybe not the best for typing purposes (I can't make it a protected variable because we intend to use it outside of this class).
Also, the approach doesn't really work for atoms_state vs. particles_state so I am still thinking about that.
|
|
||
| from nomad.datamodel.datamodel import EntryArchive | ||
| from structlog.stdlib import BoundLogger | ||
| if not TYPE_CHECKING: |
There was a problem hiding this comment.
This seems to be a general problem, I need to ask someone
| get_composition_recurs(system=system_parent, atom_labels=atom_labels) | ||
| labels = [] | ||
| if system_parent.cell is not None: | ||
| if system_parent.cell[0].name == 'AtomicCell': |
There was a problem hiding this comment.
This is just a temporary solution to make sure the rest works.
I think now that we can use some method of an archive section called m_xpath to grab the section by name of the parent class...see get_sibling_section() in utils.py
More generally I am also considering making an intermediate parent for AtomicCell and ParticleCell to share particle-specific quantities and functionalities that we don't want in cell...
| self.elemental_composition = sec_chemical_formula.m_cache.get( | ||
| 'elemental_composition', [] | ||
| ) | ||
| if any(cell.name == 'AtomicCell' for cell in self.cell): |
There was a problem hiding this comment.
This approach seems fine to me, I just generalized in case later we have some case with both atomic and particle cells (although this seems far away at this point)
There was a problem hiding this comment.
I agree. Keeping it as general as possible for as long as possible will make possible extensions later easier, even if we don't have concrete ideas about them now.
|
We do also need to consider whether a straight copy of the ase Atoms class is a sustainable/robust approach. There are many things to consider. I started investigating, but there is more work to do before I have a clear idea |
| 'The class does not have atoms_state or particles_state' | ||
| ) | ||
|
|
||
| def get(self, key, logger=None): |
There was a problem hiding this comment.
This is the crux of the implementation of getting the labels from either type of cell. Still not sure if there is a better way
There was a problem hiding this comment.
I think we do need to be able to differentiate at various points, e.g., to selectively skip certain normalizations. I've been trying to come up with a more elegant way than constantly looking for the cell type, but I'm not positive there is one at this point.
| @@ -552,7 +552,16 @@ def normalize(self, archive: 'EntryArchive', logger: 'BoundLogger') -> None: | |||
| ) | |||
|
|
|||
|
|
|||
| class AtomsState(Entity): | |||
| class State(Entity): | |||
There was a problem hiding this comment.
Didn't end up using this after all, but I left it for now in case we need it. But will remove before merge if not
| def normalize(self, archive: 'EntryArchive', logger: 'BoundLogger') -> None: | ||
| super().normalize(archive, logger) | ||
|
|
||
| # Set the name of the section | ||
| self.name = self.m_def.name if self.name is None else self.name | ||
|
|
||
|
|
||
| # TODO Consider changing name to BeadCell or CGBeadCell, only using "particle" for the more abstract reference to atoms or beads |
There was a problem hiding this comment.
@Bernadette-Mohr how would you feel about this type of naming?
There was a problem hiding this comment.
That makes a lot of sense!
Distinctive, clear naming goes a long way toward well-structured, understandable code.
|
@Bernadette-Mohr This is mostly working now (in terms of the tests for atomic cell), but I haven't adjusted the parser branch yet. I will try to do that tomorrow. Just take a look what I did and let me know if you think it makes sense |
@Bernadette-Mohr