Skip to content

Fix check for dropping to single cell in volume render#292

Open
FHusko wants to merge 1 commit intoSWIFTSIM:masterfrom
FHusko:master
Open

Fix check for dropping to single cell in volume render#292
FHusko wants to merge 1 commit intoSWIFTSIM:masterfrom
FHusko:master

Conversation

@FHusko
Copy link
Copy Markdown

@FHusko FHusko commented Feb 5, 2026

The volume render was sometimes missing particles altogether because their smoothing kernels did not encompass any cell centers. This is due to an incorrect one-half factor instead of sqrt(3) / 2 when checking how big the kernel is relative to the cell sizes.

I did not modify the scatter_limited_z function, since that one isn't used at the moment and is not working anyways as far as I am aware.

Updated the calculation for drop_to_single_cell to use a different formula based on the square root of 3.
@kyleaoman
Copy link
Copy Markdown
Member

Thanks for catching this. For the scatter_limited_z case, my intuition says that it would be:

drop_to_single_cell = pixel_width * np.sqrt(2.0) / 2.0
drop_to_single_cell_z = pixel_width_z * 0.5

but that's just at a glance. I haven't studied this part of the code carefully, and unfortunately there's nothing in the test suite covering this function as far as I can tell. If you feel up to adding a test for that it's very welcome...

@FHusko
Copy link
Copy Markdown
Author

FHusko commented Feb 9, 2026

I think the check in scatter_limited_z would need to have the ratio of cell sizes in there somewhere (since the cell size along x and y is different from z). But perhaps we should worry about that once the function itself is working?

@kyleaoman
Copy link
Copy Markdown
Member

I've no strong objection to fixing this bug without fixing the special case at the same time, in that case could you open an issue describing what still needs to be looked at in future? The code comment is ok, but easily lost/overlooked.

For this bugfix I still think that it would be worth having a regression test. Could use a single particle carefully placed so that it gets missed when the bug is present, but then gets included when the bug is fixed. Not sure how familiar you are with pytest, if you need some guidance just let me know.

@kyleaoman kyleaoman added bug Something isn't working visualisation Visualisation-related issues labels Feb 9, 2026
@FHusko
Copy link
Copy Markdown
Author

FHusko commented Feb 9, 2026

I'm not sure what's wrong with scatter_limited_z, @JBorrow will probably know.

I'll update here once I've done something on a regression test.

@kyleaoman
Copy link
Copy Markdown
Member

@FHusko don't suppose you've had a chance to write a test for this?

@FHusko
Copy link
Copy Markdown
Author

FHusko commented Mar 23, 2026

@kyleaoman Not yet, sorry! I also got ill recently which set me back a bit.

@kyleaoman
Copy link
Copy Markdown
Member

No problem, sorry to hear you haven't been well - no rush of course, might just send the occasional reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working visualisation Visualisation-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants