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

Apply foreground color to minor ticks #2305

Merged
merged 3 commits into from
Aug 5, 2022

Conversation

Carifio24
Copy link
Member

There is currently a bug where any minor ticks displayed will not match the user's global settings. (Note that this isn't an issue with the default settings, since the labels are black either way.) One prominent example is the scientific notation tick marks that are sometimes used when displaying an axis in log mode. See below for an example:

log-label-colors

This occurs because we only set major tick labels to match the given color in set_foreground_color (see the matplotlib documentation here; the default value for which is "major"). This PR fixes the issue by setting which="both").

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #2305 (eea1ea0) into main (139d88d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2305   +/-   ##
=======================================
  Coverage   88.06%   88.06%           
=======================================
  Files         247      247           
  Lines       23476    23476           
=======================================
  Hits        20673    20673           
  Misses       2803     2803           
Impacted Files Coverage Δ
glue/viewers/matplotlib/mpl_axes.py 90.24% <100.00%> (ø)

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

@@ -4,6 +4,8 @@ Full changelog
v1.4.0 (2022-05-31)
-------------------

* Fixed a bug where minor tick marks were not respecting the settings colors. [#2305]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to 1.6.0? (I am going to work on a way to avoid us having to do this in future)

@Carifio24
Copy link
Member Author

@astrofrog I've rebased and moved the changelog entry to the 1.6.0 section.

@astrofrog
Copy link
Member

Could you check the codestyle failure with tox -e codestyle - thanks!

@Carifio24
Copy link
Member Author

Hmm - it looks like the codestyle failure is actually coming from the main branch. Taking a quick look there, the codestyle check went from succeeding to failing after #2300 was merged - although codestyle passed in that PR and didn't change any of the offending files. I think the culprit is the E275 update described here.

@astrofrog astrofrog merged commit 06374b6 into glue-viz:main Aug 5, 2022
@dhomeier
Copy link
Collaborator

dhomeier commented Aug 8, 2022

any of the offending files. I think the culprit is the E275 update described here.

Thanks for identifying this @Carifio24 . The codestyle is fixed in #2308 now.

@astrofrog astrofrog added the bug label Oct 13, 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