Conversation
|
I agree with all of the changes and think this version of shear sampling is fine. Maybe we should try adding the stretch rather than scaling it? I'm happy to look at if there are any major differences. |
|
I'm pretty happy with the current form given that I convinced myself that the |
|
Yes, this does fix #24 , seems to be working as expected, except the starting max shear size should maybe be multiplied by 1.0 instead of 0.2. |
|
Can you check the latest version and confirm that the default shear step is large enough? If so, I'm ready to merge, unless you have additional concerns. |
|
Checked and the step size is sufficient now thanks. Seems to be working as expected so I have no other concerns. |
|
Great. I'll merge |
Fix scaling of cell steps. Improve documentation of these parameters.
Definitely use default scale cell shear steps that are multiplied by typical length scale.
Consider whether shear steps need to be circular in plane of two cell vectors, or whether current elliptical is OK.
Consider whether stretch steps, which multiply existing vectors, suffer from same issue that make it wrong to multiply cell volume by a max percentage.Pretty confident that the use ofexp(rv)for stretch steps makes it reversible. Consider using the same approach for volume steps. Decided to leave that for a future PR, and created #28 to remind us.closes #26