Skip to content

Conversation

@bernstei
Copy link
Collaborator

@bernstei bernstei commented Jun 18, 2025

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 of exp(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

@bernstei bernstei mentioned this pull request Jun 18, 2025
@VGFletcher
Copy link

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.

@bernstei
Copy link
Collaborator Author

I'm pretty happy with the current form given that I convinced myself that the exp(rv) form is reversible. Is it true that this branch also fixes #24 for you?

@VGFletcher
Copy link

VGFletcher commented Jun 20, 2025

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.

@bernstei
Copy link
Collaborator Author

bernstei commented Jun 20, 2025

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.

@VGFletcher
Copy link

Checked and the step size is sufficient now thanks. Seems to be working as expected so I have no other concerns.

@bernstei
Copy link
Collaborator Author

Great. I'll merge

@bernstei bernstei merged commit a645804 into main Jun 23, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cell step size scaling

3 participants