Reduce rendered images without considering opacity

[biboyd]

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

yt/yt/visualization/volume_rendering/render_source.py

Lines 535 to 537 in 6d121f5

	if not self.transfer_function.grey_opacity:
	image[:, :, 3] = 1
	return image

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.

yt/yt/utilities/amr_kdtree/amr_kdtools.py

Lines 25 to 34 in 6d121f5

	ta = 1.0 - front[:, :, 3]
	np.maximum(ta, 0.0, ta)
	# This now does the following calculation, but in a memory
	# conservative fashion
	# image[:,:,i ] = front[:,:,i] + ta*back[:,:,i]
	image = back.copy()
	for i in range(4):
	np.multiply(image[:, :, i], ta, image[:, :, i])
	np.add(image, front, image)
	return image

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.

[welcome]

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!

[chrishavlin]

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

[biboyd]

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

[neutrinoceros]

This seems sound, but we should probably add a test too. In the mean time here are a couple suggestions

[cphyc]

@yt-fido test this please

[chrishavlin]

@yt-fido test this please

[chrishavlin]

This looks good to me. Apologies for the very delayed review here, @biboyd

@neutrinoceros I'm not certain what could be added for a test since the changes here only have an effect when running with MPI. any ideas?

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

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

Ya, that's what I meant (but wasn't positive about it).

So IMO this PR is OK as it is.

Outdated

[neutrinoceros]

Thanks for clarifying. Feel free to merge then.

[welcome]

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