Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mistake in BlobTest_3D example #34

Closed
sperneder opened this issue Jun 22, 2023 · 3 comments
Closed

Mistake in BlobTest_3D example #34

sperneder opened this issue Jun 22, 2023 · 3 comments
Assignees

Comments

@sperneder
Copy link

Hi SWIFT team,

I was trying to use the makeIC.py file from the BlobTest_3D example for a different simulation when I noticed a mistake in the function generate_bcc_lattice:

def generate_bcc_lattice(num_on_side, side_length=1.0):
cube = generate_cube(num_on_side // 2, side_length)
mips = side_length / num_on_side
positions = np.concatenate([cube, cube + mips * 0.5])
return positions

The function should create a bcc lattice by generating a grid, and adding a second grid that is shifted by a half step in all 3 directions. However in this case the half step mips = side_length / num_on_side is calculated wrong, which then results in the second grid being moved too little.

A 2D plot for a grid generated with this function (with num_on_side = 10) looks like this:

bcc_lattice_wrong

A possible solution would be to change the line mips = side_length / num_on_side to mips = side_length / (num_on_side // 2).
This would result in a grid that looks like this:

bcc_lattice_right
@JBorrow
Copy link
Member

JBorrow commented Jun 25, 2023

Thanks! You're totally correct here. I've looked at some of our private checkouts of the script that we've used for various works, and they have this issue corrected; it must just be something that we've forgotten to push upstream at some point. I will address this in the GitLab version of the repo (our GitHub repo is just a copy of the GitLab one) and close this when appropriate.

Thanks again for reporting this. It's very helpful! Glad you're making use of the script; I would suggest that for now you go ahead and use your fixed version that correctly creates the BCC lattice.

@JBorrow
Copy link
Member

JBorrow commented Jun 25, 2023

The GitLab merge request is here: https://gitlab.cosma.dur.ac.uk/swift/swiftsim/-/merge_requests/1733

pwdraper pushed a commit that referenced this issue Jun 26, 2023
@JBorrow
Copy link
Member

JBorrow commented Jun 28, 2023

Thank you! We've now fixed this in the main codebase. The changes will synchronise with the master branch on GitHub tonight.

https://gitlab.cosma.dur.ac.uk/swift/swiftsim/-/merge_requests/1733/diffs#note_49912

Thanks again for reporting this!

@JBorrow JBorrow closed this as completed Jun 28, 2023
pwdraper pushed a commit that referenced this issue Jun 29, 2023
Fix GitHub Issue #34

See merge request swift/swiftsim!1733
pwdraper pushed a commit that referenced this issue Sep 7, 2023
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

No branches or pull requests

2 participants