Skip to content

Cell step param scaling#27

Merged
bernstei merged 4 commits intomainfrom
cell_step_param_scaling
Jun 23, 2025
Merged

Cell step param scaling#27
bernstei merged 4 commits intomainfrom
cell_step_param_scaling

Conversation

@bernstei
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

@bernstei
Copy link
Copy Markdown
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

2 participants