search

SWIFTSIM / SWIFT

Modern astrophysics and cosmology particle-based code. Mirror of gitlab developments at https://gitlab.cosma.dur.ac.uk/swift/swiftsim
http://www.swiftsim.com
GNU Lesser General Public License v3.0
88 stars 58 forks source link

Mistake in BlobTest_3D example #34

Closed sperneder closed 1 year ago

sperneder commented 1 year ago

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:

https://github.com/SWIFTSIM/SWIFT/blob/2287ed51f2ff9555e22ae81983c1708f73644952/examples/HydroTests/BlobTest_3D/makeIC.py#L35-L42

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 commented 1 year ago

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 commented 1 year ago

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

JBorrow commented 1 year ago

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!