Navigation Menu

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

“Size” ignored if placed before fontproperties #16389

Closed
shawnchenx6 opened this issue Feb 2, 2020 · 7 comments · Fixed by #16694
Closed

“Size” ignored if placed before fontproperties #16389

shawnchenx6 opened this issue Feb 2, 2020 · 7 comments · Fixed by #16694

Comments

@shawnchenx6
Copy link
Contributor

shawnchenx6 commented Feb 2, 2020

Bug report

Bug summary

In as.text, ax.set_title and all the functions realated to displaying characters on the figure,
the arguement size is ignored if it is placed before fontproperties.

Code for reproduction

data = np.random.randn(10000)
plt.hist(data, bins=40, facecolor="blue", edgecolor="black", alpha=0.5)
plt.xlabel("value", fontproperties='SimHei',size=20  ) # this will work
plt.ylabel("counts",size=20, fontproperties='SimHei')  # this doesn't
plt.show()

This issue is replicated with ipython (%pylab) and jupyter-notebook

Actual outcome

# If applicable, paste the console output here
#
#

Expected outcome

Matplotlib version

  • Operating system: win10 1909
  • Matplotlib version: Version :3.1.2 , Build : py36_1 , conda-forge
  • Matplotlib backend (print(matplotlib.get_backend())): module://ipykernel.pylab.backend_inline
  • Python version: 3.6.10
  • Jupyter version (if applicable): version 1.0.0
  • Other libraries:

conda install -c conda-forge matplotlib

@shawnchenx6 shawnchenx6 changed the title Don’t “Size” ignored if replaced before fontproperties Feb 2, 2020
@shawnchenx6 shawnchenx6 reopened this Feb 3, 2020
@shawnchenx6 shawnchenx6 changed the title “Size” ignored if replaced before fontproperties “Size” ignored if placed before fontproperties Feb 3, 2020
@timhoffm
Copy link
Member

timhoffm commented Feb 5, 2020

Somehow update() gets called with both values even though Text() holds fontproperties as explicit parameter but size is handled via **kwargs. Since the passed arguments of update() are processed sequentially, order matters here.

If fontproperties is used with a string, a FontProperties instance with non-given defaults (including size) is created. That will overwrite the explicit size in the second example.

IMHO expicit font-related keywords like size should have precedence over fontproperties.

This is a good first issue to dig into. Please include a test based on the above example to validate the fix.

@tacaswell tacaswell added this to the v3.3.0 milestone Feb 10, 2020
@tacaswell
Copy link
Member

However, if we get passed an actual FontProperties objects (not a string we implicitly up-convert) the precedence is not completely clear. I can see an argument for in that case the order of the kwargs mattering and the last one winning, however in the case of a string input (like this example) it is pretty clear that the size kwarg should have higher priority. Given that is is better to not have behavior like that depend on the exact types, size should always win.

Which is a very long way of saying "I agree with @timhoffm".

@asongtoruin
Copy link
Contributor

I'd be happy to take a look at this if you wouldn't mind?

From a quick dig around (and @timhoffm's comment) I think it's coming from Artist.update()

def update(self, props):
"""
Update this artist's properties from the dict *props*.
Parameters
----------
props : dict
"""
ret = []
with cbook._setattr_cm(self, eventson=False):
for k, v in props.items():
k = k.lower()
# White list attributes we want to be able to update through
# art.update, art.set, setp.
if k == "axes":
ret.append(setattr(self, k, v))
else:
func = getattr(self, f"set_{k}", None)
if not callable(func):
raise AttributeError(f"{type(self).__name__!r} object "
f"has no property {k!r}")
ret.append(func(v))
if ret:
self.pchanged()
self.stale = True
return ret

The simplest solution I can think of would be to pop fontproperties from the props dict to ensure set_fontproperties is run before any of the other functions. It's not particularly elegant, but it would ensure that values given by the other kwargs would always take precedence.

@anntzer
Copy link
Contributor

anntzer commented Feb 12, 2020

See #16328 where I'm basically trying to get rid of this kind of kwargs reordering...

I think the proper solution would be for FontProperties to properly keep unset properties as being, well, unset, and not overwriting already existing properties on the text object.

Also, I guess this raises some other questions, e.g. in

text = ax.text(...)
text.set_fontproperties("size=24")  # or the corresponding FP object
text.set_fontproperties("SimHei")  # or the corresponding FP object

should the second call reset the size to the rc default?

(Note that in the base case here, passing family="SimHei" should work just fine, and will in fact work better if the font family contains a dash...)

@RiazCharania
Copy link

Is anyone else currently working on this? If not I'll give it a try.

@timhoffm
Copy link
Member

timhoffm commented Mar 4, 2020

@RiazCharania go for it. The first step is to clarify the expected behavior. See #16389 (comment) and #16389 (comment). I that you provide a short summary defining the behavior for all cases before implementing.

@HarrisonFok
Copy link

We'll give it a try too

omarchehab98 added a commit to CSCD01-team20/matplotlib that referenced this issue Mar 6, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
omarchehab98 added a commit to CSCD01-team20/matplotlib that referenced this issue Mar 6, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389

Previously, setting Text's FontProperties overwrites any old properties
After the change, setting Text's FontProperties merges with old
properties
omarchehab98 added a commit to CSCD01-team20/matplotlib that referenced this issue Mar 6, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
omarchehab98 added a commit to CSCD01-team20/matplotlib that referenced this issue Mar 7, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
DennisTismenko pushed a commit to CSCD01-team20/matplotlib that referenced this issue Mar 13, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
DennisTismenko pushed a commit to CSCD01-team20/matplotlib that referenced this issue Mar 13, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
DennisTismenko pushed a commit to CSCD01-team20/matplotlib that referenced this issue Mar 13, 2020
Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants