FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make Text._get_layout simpler to follow. by anntzer · Pull Request #12951 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Make Text._get_layout simpler to follow. - #12951

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:get_layout
Jan 13, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Make Text._get_layout simpler to follow.#12951
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:get_layout

Conversation

anntzer commented Dec 9, 2018

Copy link
Copy Markdown
Contributor
  • Instead of constructing and accessing multidimensional arrays whs and
    horizLayout by (opaque) index, construct separate lists with more
    meaningful names.
  • Define xmin/xmax/ymin/ymax/width/height closer to their place of use.
  • Make computation of offsetx and offsety more symmetric between
    rotation_mode == "anchor" and == "default".

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

timhoffm reviewed Dec 9, 2018
View reviewed changes

timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also would benefit from renaming some few-character variables, e.g. d -> descent

Comment thread lib/matplotlib/text.py
# Metrics of the last line that are needed later:
baseline = (h - d) - thisy

if i == 0:

timhoffm Dec 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if i == 0:
if thisy == 0: # first line

gets rid of the need for enumerate().

anntzer Dec 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's not immediately obvious that thisy cannot be zero for the second line too even if the first line is empty (it's true, but only thanks to the h = max(h, lp_h)/d = max(d, lp_d) lines) so I think leaving it as it is is clearer.

timhoffm Dec 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just by common sense, the vertical position thisy must increase also for empty lines. So it should only be zero before handling the first line.

But ok, if you want to leave it.

anntzer Dec 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think common sense applies to this function (or to a lot of matplotlib, sadly). (e.g. if you look at the way center_baseline is defined a bit below, it looks weirdly different between anchor and non-anchor rotations...)

anntzer Jan 2, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Funnily that got fixed by #13029 :)

Comment thread lib/matplotlib/text.py
horizLayout[i] = thisx, thisy, w, h
thisy -= max(min_dy, (h - d) * self._linespacing)

xs.append(thisx) # == 0.

timhoffm Dec 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Alterantively

xs = [0.] * len(ys)

outside of the loop.

anntzer Dec 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the symmetry of handling between xs and ys is nicer.

timhoffm Dec 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

IMO xs = [0.] * len(ys) is much clearer about xs than secretly constructing a list of zeros in the loop. The loop should only need to bother about the quantities that actually depend on the lines. But not going to argue about this.

anntzer commented Dec 9, 2018

Copy link
Copy Markdown
Contributor Author

The single-character variables are "inside the loop", the longer ones are the one that outlive the loop.

tacaswell added this to the v3.1 milestone Dec 9, 2018

timhoffm commented Dec 9, 2018

Copy link
Copy Markdown
Member

The single-character variables are "inside the loop", the longer ones are the one that outlive the loop.

Which still doesn't make the single-character variables readable.

timhoffm approved these changes Dec 9, 2018
View reviewed changes

timhoffm left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Accepting as an incremental improvement, even though I see room for further improvement.

anntzer commented Dec 9, 2018

Copy link
Copy Markdown
Contributor Author

The single-letter variables are defined as

w, h, d = renderer.get_text_width_height_descent(...)

so their meaning is pretty clear IMO (in the context).

timhoffm commented Dec 9, 2018

Copy link
Copy Markdown
Member

That's fine in a narrow scope like a list comprehension or a few-lines for. Single letter variables should just not live for more than 20 lines. And in particular not multiple different ones at the same time.

anntzer commented Jan 2, 2019

Copy link
Copy Markdown
Contributor Author

Additionally factored out offsets computation, following #13029.

anntzer mentioned this pull request Jan 12, 2019
Merged
6 tasks
Make Text._get_layout simpler to follow.
b140c65 
- Instead of constructing and accessing multidimensional arrays whs and
  horizLayout by (opaque) index, construct separate lists with more
  meaningful names.
- Define xmin/xmax/ymin/ymax/width/height closer to their place of use.
- Make computation of offsetx and offsety more symmetric between
  rotation_mode == "anchor" and == "default".
anntzer mentioned this pull request Jan 12, 2019
Merged
6 tasks
tacaswell merged commit 4cdd071 into matplotlib:master Jan 13, 2019
anntzer deleted the get_layout branch January 13, 2019 21:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

topic: text

Projects

None yet

Milestone

v3.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL