-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
Discussion with Adham led me to realise primitive cell configurations not included in the code. Please hold off review for now. Thanks! |
Added features:
Please review. Thanks. |
A documentation (for sphinx) is not yet written. I believe it is better to write a section for a set of tools under the tools module (lattice, crystal) once they are finalised. |
targets = (v1, v2, v3) | ||
|
||
# cosine angles between pairs of the target primitive vectors | ||
target_cosines = numpy.abs([cosine_two_vectors(vec1, vec2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kitchoi, a question: Why numpy.abs() here (and L81)? This seems to interpret some "geometrically distinct" sets of vectors similar. I don't have an exact example in mind, but because cosine changes sign at pi/2, I think there might be a problem with some sets of vectors that have angles pi/2+c and pi/2-c, with c constant. Cosines of those angles would be the same. Maybe there's something I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tuopuu thanks for checking with care! The absolute value here (and in the other functions) is to take care of the possibility of flipping a vector. Two pairs of vectors with angles = (pi/2+c) and (pi/2-c) respectively are equivalent to flipping one of the two vectors by 180 degree. These two sets of primitive vectors would generate the same lattice type. An example of this is a hexagonal primitive cell with a 60 degree or 120 degree between the base vectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, @kitchoi. It makes sense now to me. A 120 degree hexagonal lattice then equals a 60 degree one that is rotated 180 degrees around one of the main axises. Sorry, but I'll have to do something else now but I'll continue the review later this evening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tuopuu. But now I can see that the docstring is leading to you think that the function should differentiate a hexagonal cell of 60 degree and 120 degree because I wrote there "geometrically similar". I can imagine a user may have the same surprise when calling this function independently. Maybe this is clearer?
Return True if a set of primitive vectors ``p1``, ``p2``, ``p3``
describe a primitive cell that is geometrically similar to another
primitive cell described by (+/- ``v1``, +/- ``v2``, +/- ``v3``)
(I struggled to make it look pretty, hmm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the function only tests for angles and lengths, as it currently is, then the lattice type will be invariant under rotation, translation or reflection. A mathematical term for this collection of "geometrically similar" lattices is often called an "equivalence class". I think this is a really useful feature, because generally users don't often have the exact same configuration of the lattice that we have chosen as the representative of each lattice type. Currently SimPhoNy does not offer means to rotate or reflect the lattices, only translation is allowed by setting the origin parameter in the factory functions.
Maybe we should change the docstrings once more to describe this behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, translation invariance is not tested here as there is no comparison of the origin parameters. Primitive vectors themselves, of course, do not hold this information. So, please, ignore that part from my last comment.
Maybe the docstrings could say something along the lines of "Return True if a set of primitive vectors p1, p2, p3 describe a primitive cell that is similar under rotation and/or reflection to the cell described by v1, v2, v3."
@kitchoi, I left a few minor comments between the lines. Nice looking code generally. This package will be very useful to crystallographers and lattice-Boltzmann enthusiasts. 👍 |
Thanks @tuopuu! All fixed. Please let me know if this is good to merge. Thanks. |
@kitchoi, looks good. 👍 I would have liked to still improve the docstrings in |
Add tools module and lattice_tools
Added lattice_tools as
simphony.tools.lattice_tools
- please comment if you think a different naming or hierarchy is better)
Added tests for the lattice_tools
- It takes a while because
hypothesis
tries to randomly construct many different primitive cells to catch edge cases; most of the time is spent on generating many primitive cell objects, not on callinglattice_tools
Added
hypothesis
to dev_requirements.txt- hugely useful for development (that's how I found out about #245)
Please play with it with your use cases.
Related to #240