yt
yt copied to clipboard
Reduce rendered images without considering opacity
PR Summary
Issue
This addresses issue #4871, where volume renders of multiple fields with MPI would only produce a portion of the second field in the final image.
I believe I have found where this bug occurs. It mainly has to do with the combining of image arrays in yt/utilities/amr_kdtree/amr_kdtools.py:receive_and_reduce()
and how the alpha channel is set in yt/visualization/volume_rendering/render_source.py:VolumeSource.finialize_image()
After the first source is rendered, the last step of finalize_image()
sets the alpha channel of the image array to 1
's
https://github.com/yt-project/yt/blob/6d121f58543bedb1f587492cd0601a69f06a736a/yt/visualization/volume_rendering/render_source.py#L535-L537
This causes issues when the second source is combining the image arrays in recieve_and_reduce()
as these two arrays are combined by scaling the back
image by ta
, 1
minus the front
alpha channel. Since the alpha channel was already set by the first source, ta
is a zeros array. This eliminates any contribution of back
to the final image.
https://github.com/yt-project/yt/blob/6d121f58543bedb1f587492cd0601a69f06a736a/yt/utilities/amr_kdtree/amr_kdtools.py#L25-L34
Solution
My proposed solution is to add a pathway to combine image arrays without the consideration of opacity. This would be used when transfer_function.grey_opacity=False
. In this case, front
and back
don't have any relevance and we can simply add the two image arrays.
PR Checklist
- [ ] New features are documented, with docstrings and narrative docs
- [ ] Adds a test for any bugs fixed. Adds tests for new features.
Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!
thanks for the pull request! I'll review in detail soon, but you're getting a lot of real test failures that you could fix by making your new use_opacity
argument a keyword argument instead (and ideally the default value would be such that it does not change current behavior if you do not set the keyword argument).
Thanks! Made the change to a keyword and now reduce_tree_images
should default to the original behavior
@yt-fido test this please
@yt-fido test this please
Do you mean we don't have any form of continuous testing with MPI yet ? if so, I'd agree to leave it out of this PR, but we should probably open issue for it.
Do you mean we don't have any form of continuous testing with MPI yet ? if so, I'd agree to leave it out of this PR, but we should probably open issue for it.
Ya, that's what I meant (but wasn't positive about it).
So IMO this PR is OK as it is.
Thanks for clarifying. Feel free to merge then.
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! :fireworks: