Update art3d.py to address strange behavior of depthshading on 3D scatterplots with close points

[nhansendev]

trafficstars

PR Summary

This update changes the depthshade behavior from the original normalization method to an improved version that behaves well even when points are closely spaced. This addresses issue 22861

I'm not clear on how to run the appropriate pytest unit tests for this file (if any), so I would appreciate some guidance there.

PR Checklist

Tests and Styling

• [ ] Has pytest style unit tests (and pytest passes).
• [ ] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

• [ ] New features are documented, with examples if plot related.
• [ ] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
• [ ] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
• [ ] Documentation is sphinx and numpydoc compliant (the docs should build without error).

[nhansendev]

I think I've addressed the other issues, but am still getting image comparison failures. Is this to be expected when making changes related to how plots are displayed?

[oscargus]

Is this to be expected

Yes, it is expected. The RMS numbers are quite small, so only a number of pixels.

I will let someone else have a proper say on this, but I believe that we should provide a fallback method for persons relying on the previous appearance. Unless it is obvious that the previous was always incorrect.

[oscargus]

If you please can provide the resulting figures from e.g. #22861 after applying the changes here, it would simplify the review.

[nhansendev]

If you please can provide the resulting figures from e.g. #22861 after applying the changes here, it would simplify the review.

I've attached some screenshots below showing the updated behavior.

This is with no alpha input, with depth-shading on. You can see how the alpha becomes uniform when depth is uniform (1st image), then fades proportionally to draw depth, including when draw depth reverses (2nd vs 3rd image):

When there is an alpha input (with depthshading on or off), the result is that the slight rotation of the plot no longer results in a sudden flip of alpha values as the depth order reverses:

Note that this behavior is only implemented for Path3DCollection since the bug is related to 3D scatterplots, but may be applicable to other collections if needed.

[nhansendev]

I feel like I should explain the comment "# Solid near, transparent far, solid default" further: depending on the equation used for calculating "sats" you can change the behavior of the depth-shading.

The meaning is as follows: Solid near: shallower datapoints are more/completely opaque Transparent far: deeper datapoints become increasingly transparent Solid default: if everything is near equal depth then everything will be opaque

I felt that the other three versions were further from the desireable/existing depth-shading behavior, but could be interesting to include as alternative modes.

Please let me know if you think "mode selection" is a worthwhile inclusion. Mode selection could also allow the old depth-shading behavior to be selected if desired.

[airalcorn2]

Thanks for this. This behavior was driving me crazy.

[tacaswell]

This is cool work! A couple of requests:

• can we use an enum (or strings) rather than integers for the modes?
• can you add a whats_new to show this off?
• can you add tests?

I have pushed this to the 3.8 milestone.

[nhansendev]

• can we use an enum (or strings) rather than integers for the modes?

Definitely, though I'm not sure what appropriate string representations would be. My previous convention was to summarize behavior as follows: "STS": Solid when shallow, Transparent when deep, Solid default, "STT": Solid when shallow, Transparent when deep, Transparent default, etc.

• can you add a whats_new to show this off?
• can you add tests?

Are there guidelines for these/examples I could reference?

Thanks for the suggestions.

[melissawm]

Hi @Obliman - For the tests, you can take a look at this file which contains the tests for art3d.py. For the what's new, here's the documentation. Let me know if you have other questions. Cheers!

[jklymak]

Noted as draft until tests are fixed. Thanks!

[scottshambaugh]

Hi @nhansendev, are you still interested in finishing up this bugfix/feature? Thanks!

[nhansendev]

Hi @nhansendev, are you still interested in finishing up this bugfix/feature? Thanks!

Thanks for the reminder, I've made some updates.

I've thought about the changes I had proposed and the whole "different modes" thing was just unnecessarily complicated/probably not that useful. I've since simplified it to be three kwargs:

1. inverted: whether to progress from opaque to transparent (default) or the opposite
2. min_alpha: the smallest allowable alpha value (highest transparency)
3. legacy: whether to use the old algorithm

I've added the "what's new" document and updated other classes to provide access to the user when creating scatter plots or otherwise interacting with Patch3DCollection/Path3DCollection objects.

Edit: one thing I'm still unsure about is whether there are any tests that need to be set up for this. Can anyone provide guidance, please?

[oscargus]

Yes, I think that there should be some tests for this to make sure that possible later changes does not break the behavior.

One suggestion is to add an image comparison test with a number of subplots, each subplot illustrating some combination of the new behavior.

Looking at the code coverage, I can imagine a 2 x 2 subplot where each combination of legacy/new and inverted/non-inverted is plotted. (Right now, only new and non-inverted seems to be tested.)

For min-alpha one can consider something similar or simply checking the alphas in the resulting collection to confirm that they are as expected (if one can avoid image tests that is slightly better since storing images leads to a rather large repo...).

[oscargus]

Oh, and for "completeness", one can also call the new get_* methods as part of some test. Simply confirming that they return the expected value.

[tacaswell]

This is breaking tests that were expecting the old behavior. We either need to manually set the legacy behavior in the tests and add an API change note or we need to set the default to set the default value of depthshade_legacy to True.

Rather than adding a bunch of parameters named depthshade_XXX, we should think of ways to collapse them (either bool + dict or relying on the type of depthshade (suggeted by @ksunden ).

Moving this to 3.10 and should talk about this on the call as soon as 3.9 is branched.