Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force line collection to always respect colormap #2299

Merged
merged 1 commit into from
Oct 16, 2022

Conversation

Carifio24
Copy link
Member

The scatter viewer currently has a bug (noticed by @victoriaono) where the line collection that connects points will only obey the selected colormap if that colormap was set before a line was first displayed on the viewer. Additionally, if a fixed color is set after the line displays, subsequent colormap usage will not be respected. See the video below for an example.

scatter-line-bug-2022-05-21_18.14.18.mp4

This PR creates consistent behavior by calling set_color(None) inside the line collection's set_linearcolor method. The reason for this has to do with how the behavior is implemented inside matplotlib. Whenever the colormap mode is Fixed, we call set_linearcolor with color=self.state.color here. This call's the artist's set_color method, which ultimately calls set_facecolor and sets _original_facecolor to that color.

However, when cmap_mode is set to Linear, we call set_mpl_artist_cmap here without a color argument, which doesn't doesn't call set_color, and so _original_facecolor remains as the solid color. The artist's _edge_is_mapped attribute is set to false if _original_edgecolor has a color value, which means that the edgecolors of the artist are set to the original edgecolor, rather than the colormapped colors. This is why things work if we set the colormap before the line first displays - we haven't made a call to the line collection's set_color method yet, so _original_facecolor is still None. By calling set_color(None), we reset _original_facecolor to None, allowing the line collection edges to be colormapped.

@codecov
Copy link

codecov bot commented May 21, 2022

Codecov Report

Base: 88.09% // Head: 88.06% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (06374b6) compared to base (a59ef81).
Patch has no changes to coverable lines.

❗ Current head 06374b6 differs from pull request most recent head 2d27081. Consider uploading reports for the commit 2d27081 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2299      +/-   ##
==========================================
- Coverage   88.09%   88.06%   -0.04%     
==========================================
  Files         247      247              
  Lines       23563    23476      -87     
==========================================
- Hits        20758    20673      -85     
+ Misses       2805     2803       -2     
Impacted Files Coverage Δ
glue/__init__.py 66.66% <0.00%> (-3.71%) ⬇️
glue/core/link_helpers.py 84.61% <0.00%> (-1.28%) ⬇️
glue/viewers/matplotlib/qt/widget.py 77.27% <0.00%> (-1.16%) ⬇️
glue/core/data.py 91.71% <0.00%> (-0.74%) ⬇️
glue/core/layer_artist.py 80.00% <0.00%> (-0.23%) ⬇️
glue/core/visual.py 87.38% <0.00%> (-0.23%) ⬇️
glue/utils/matplotlib.py 90.27% <0.00%> (-0.21%) ⬇️
glue/core/link_manager.py 97.64% <0.00%> (-0.13%) ⬇️
glue/core/state.py 91.34% <0.00%> (-0.04%) ⬇️
glue/viewers/matplotlib/viewer.py 94.38% <0.00%> (-0.03%) ⬇️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@astrofrog
Copy link
Member

astrofrog commented Aug 4, 2022

Looks good, can you add a changelog entry? (in the 1.6.0 section - you might need to rebase)

@Carifio24
Copy link
Member Author

@astrofrog I've rebased and added an entry in the changelog

@astrofrog
Copy link
Member

@Carifio24 - could you rebase this?

@Carifio24
Copy link
Member Author

@astrofrog I've just rebased

@Carifio24
Copy link
Member Author

Just rebasing to resolve a conflict

@astrofrog astrofrog merged commit fe7e5ff into glue-viz:main Oct 16, 2022
@dhomeier dhomeier added the bug label Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants