yt icon indicating copy to clipboard operation
yt copied to clipboard

Reduce rendered images without considering opacity

Open biboyd opened this issue 10 months ago • 4 comments

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.

biboyd avatar Apr 16 '24 17:04 biboyd

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!

welcome[bot] avatar Apr 16 '24 17:04 welcome[bot]

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).

chrishavlin avatar Apr 17 '24 14:04 chrishavlin

Thanks! Made the change to a keyword and now reduce_tree_images should default to the original behavior

biboyd avatar Apr 17 '24 16:04 biboyd

@yt-fido test this please

cphyc avatar May 06 '24 07:05 cphyc

@yt-fido test this please

chrishavlin avatar May 13 '24 16:05 chrishavlin

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.

neutrinoceros avatar May 14 '24 06:05 neutrinoceros

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.

chrishavlin avatar May 14 '24 14:05 chrishavlin

Thanks for clarifying. Feel free to merge then.

neutrinoceros avatar May 14 '24 14:05 neutrinoceros

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! :fireworks:

welcome[bot] avatar May 15 '24 14:05 welcome[bot]