-
Notifications
You must be signed in to change notification settings - Fork 74
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
Reverse preset color order #2568
Conversation
Add changelog
ff4d96f
to
103acaa
Compare
preset_colors = [preset_colors[0], preset_colors[3]] | ||
preset_colors = [preset_colors[1], preset_colors[4]] | ||
elif n_visible == 3: | ||
preset_colors = [preset_colors[0], preset_colors[2], preset_colors[3]] | ||
preset_colors = [preset_colors[1], preset_colors[2], preset_colors[4]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this surprised me at first (I expected just a change to preset_colors
), but I guess this is just to avoid the purple on the end and the purple switched sides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2568 +/- ##
==========================================
- Coverage 90.84% 90.79% -0.05%
==========================================
Files 160 160
Lines 19357 19356 -1
==========================================
- Hits 17584 17575 -9
- Misses 1773 1781 +8 ☔ View full report in Codecov by Sentry. |
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changleog failure seems to be due to a docker build failure in the CI infrastructure, not related. @kecnry cool if I hit merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restarted the failed jobs, but if they fail again for no reason, feel free to merge (just added one small suggestion - take it or leave it!)
if n_visible >= 2 and n_visible < len(preset_colors): | ||
preset_colors = [preset_colors[i] for i in preset_inds[n_visible]] | ||
elif n_visible > len(preset_colors): | ||
cmap = matplotlib.colormaps['gist_rainbow'].resampled(n_visible) | ||
preset_colors = [matplotlib.colors.to_hex(cmap(i), keep_alpha=True) for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might now read better and be harder to break in the opposite order (if n_visible > len(preset_color): ... elif n_visible > 2: ...
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I wasn't thinking about the ==
case, so doesn't really make a difference I guess 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm not changing it back!
As requested in #2558 (comment).