raise warning if geometry is out of bounds #270
Conversation
Co-authored-by: Peter C Kroon <[email protected]>
Co-authored-by: Peter C Kroon <[email protected]>
Co-authored-by: Peter C Kroon <[email protected]>
Co-authored-by: Peter C Kroon <[email protected]>
|
@pckroon this PR exploded little, but you were asking for it (sort of). 😄 I had to make a proper copy method for meta-molecule, because I realised that one of the tests of the build-file parser was compromised as the copy method of graph doesn't actually set attributes of the subclass. I think this is a network problem. Now that is fixed and the build file tests are stricter. But it appears the whole testing of Logging also needs an overhaul. |
| raise NotImplementedError(msg) | ||
| G = self.__class__(force_field=self.force_field, | ||
| mol_name=self.mol_name) | ||
| G.molecule = self.molecule |
There was a problem hiding this comment.
Not sure about this one. It seems more efficient to return a molecule instead of a copy of a molecule but then it really depends on wether you want to change the molecule attributes or not. Any ideas on a sensible default, perhaps have a argument like copy_molecule=False?
There was a problem hiding this comment.
Hmmn, I'm hesitant to add extra arguments to this method. Where is this (indirectly) called?
I would err on the side of function over speed. Wrong results are worse than slow results imo.
| """ | ||
| Raise a warning if the point of reference | ||
| for the geometry type is too close to the | ||
| origin. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| geom_def: dict | ||
| dict with entries: resname, start, stop, point, parameters | ||
| geom_type: str | ||
| one of sphere, cylinder, rectangle | ||
| """ |
| point = geom_def['parameters'][1] | ||
| if geom_type == "sphere": | ||
| radius = geom_def['parameters'][2] | ||
| if any( i >= j for i, j in zip([radius, radius, radius], point)): |
There was a problem hiding this comment.
Is checking the cardinal axes enough? Or you do need to do (point**2).sum() vs 3*radius**2?
|
|
||
| if geom_type == "cylinder": | ||
| r, z = geom_def['parameters'][2:4] | ||
| if z >= point[2] or any(r >= j for j in point[:2]): |
There was a problem hiding this comment.
See previous comment about radius
| raise NotImplementedError(msg) | ||
| G = self.__class__(force_field=self.force_field, | ||
| mol_name=self.mol_name) | ||
| G.molecule = self.molecule |
There was a problem hiding this comment.
Hmmn, I'm hesitant to add extra arguments to this method. Where is this (indirectly) called?
I would err on the side of function over speed. Wrong results are worse than slow results imo.
| with caplog.at_level(logging.WARNING): | ||
| result = polyply.src.build_file_parser.BuildDirector._base_parser_geometry(tokens, _type) | ||
| for record in caplog.records: | ||
| assert record.levelname == "WARNING" | ||
| break | ||
| else: | ||
| assert False |
There was a problem hiding this comment.
Do you want to test len(records)?
| copy_mol = meta_mol.copy() | ||
| assert list(meta_mol.nodes) == list(copy_mol.nodes) | ||
| assert meta_mol.force_field == copy_mol.force_field | ||
| assert meta_mol.molecule == copy_mol.molecule | ||
| for node in meta_mol.nodes: | ||
| for key, attr in meta_mol.nodes[node].items(): | ||
| assert attr == copy_mol.nodes[node][key] |
There was a problem hiding this comment.
This is not thorough enough: modify copy_mol (or mol) and assert the other is not affected
No description provided.