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

Simplify units.Registry.get_converter. by anntzer · Pull Request #9314 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Simplify units.Registry.get_converter. - #9314

Merged
timhoffm merged 1 commit into
matplotlib:masterfrom
anntzer:simpler-units
Feb 28, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Simplify units.Registry.get_converter.#9314
timhoffm merged 1 commit into
matplotlib:masterfrom
anntzer:simpler-units

Conversation

anntzer commented Oct 8, 2017

Copy link
Copy Markdown
Contributor

The caching of the converter was disabled 10 years ago
(be73ef4) and is unlikely to ever work
as the correct converter for ndarrays depends on the type of the
contents, not on the container, so just drop the cache-related code.

When a masked array is passed, it's OK to just get the first underlying
data, even if it is masked (as it should have the same type as the
unmasked entries), for the purpose of getting the converter.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

dstansby added this to the 2.2 (next feature release) milestone Oct 8, 2017
dstansby previously requested changes Feb 10, 2018
View reviewed changes

dstansby 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

A couple of things that I think need leaving in.

Comment thread lib/matplotlib/units.py
def get_converter(self, x):
'get the converter interface instance for x, or None'

if not len(self):

dstansby Feb 10, 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

I think it would be good to keep this in, to avoid stepping through the following logic if there aren't any convertors registered in the first place.

anntzer Feb 10, 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 cost of an instance check, a dict lookup and attempting to get the first element is quite small. For code like this I would like to see at least a microbenchmark showing at least some minor improvement (not necessarily a big one) before making it more complex than necessary.

Comment thread lib/matplotlib/units.py
converter = None
classx = getattr(x, '__class__', None)

if classx is not None:

dstansby Feb 10, 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

Shouldn't this also stay in, as there's no guarantee that unit data is a subclass of numpy array or an iterable?

anntzer Feb 10, 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

Should be handled by return self[type(x)], no?

dstansby Feb 10, 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

Ah yes

dstansby commented Feb 10, 2018

Copy link
Copy Markdown
Member

(but I think simplifying the numpy logic is a good thing to do!)

tacaswell modified the milestones: v2.2, v3.0 Feb 10, 2018

tacaswell commented Feb 10, 2018

Copy link
Copy Markdown
Member

Please hold of on merging this for 2.2.

anntzer mentioned this pull request May 19, 2018
Closed
dstansby dismissed their stale review May 21, 2018 15:56

Old review

jklymak removed this from the v3.0 milestone Jul 9, 2018

jklymak commented Jul 9, 2018

Copy link
Copy Markdown
Member

I think this should wait for the units MEP, if such is forthcoming...

tacaswell added this to the v3.1 milestone Jul 10, 2018

anntzer commented Jul 10, 2018

Copy link
Copy Markdown
Contributor Author

AFAICT this should not change any semantics.

jklymak commented Feb 27, 2019

Copy link
Copy Markdown
Member

Can this get a rebase?

I'm still nervous about any changes to units without more thorough testing, i.e. w/ pandas, datetime, pint, etc. OTOH, this seems a lot simpler, so I'm inclined to approve given that tghe existing tests do pass...

anntzer commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

rebased

anntzer mentioned this pull request Feb 27, 2019
Merged
6 tasks

jklymak commented Feb 27, 2019

Copy link
Copy Markdown
Member

... good thing we added some tests 😉

anntzer commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

good catches, should be fixed now.

jklymak approved these changes Feb 27, 2019
View reviewed changes

jklymak 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

This looks right to me. Could have an extra comment or two, for instance after the except KeyError, but I supposes its clear enough

jklymak reviewed Feb 27, 2019
View reviewed changes
Comment thread lib/matplotlib/units.py

# If x is an array, look inside the array for data with units
"""Get the converter interface instance for *x*, or None."""
if hasattr(x, "values"):

jklymak Feb 27, 2019

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

@ImportanceOfBeingErnest had some concerns about this way to check for pandas:
#11664 (comment)

anntzer Feb 28, 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

One could do something like type(x).__module__.startswith("pandas.") and hasattr(x, "values") but that should be a separate PR; this PR doesn't change the way pandas testing is done.

Simplify units.Registry.get_converter.
25cae8e 
The caching of the converter was disabled 10 years ago
(be73ef4) and is unlikely to ever work
as the correct converter for ndarrays depends on the type of the
contents, not on the container, so just drop the cache-related code.

When a masked array is passed, it's OK to just get the first underlying
data, even if it is masked (as it should have the same type as the
unmasked entries), for the purpose of getting the converter.

anntzer commented Feb 28, 2019

Copy link
Copy Markdown
Contributor Author

added comments

timhoffm reviewed Feb 28, 2019
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

This looks right, but I would be more comfortable if there were some tests.

anntzer commented Feb 28, 2019

Copy link
Copy Markdown
Contributor Author

There are already tests (as noted by Jody anbove, an earlier version of this failed them).

timhoffm merged commit cd3ed7a into matplotlib:master Feb 28, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 28, 2019
Backport PR matplotlib#9314: Simplify units.Registry.get_converter.
6d25a4a
meeseeksmachine mentioned this pull request Feb 28, 2019
Merged
anntzer deleted the simpler-units branch February 28, 2019 17:00

jklymak commented Feb 28, 2019

Copy link
Copy Markdown
Member

There are some tests. I'd still argue there are not enough, but I guess we will find out what breaks...

dstansby added a commit that referenced this pull request Feb 28, 2019
Merge pull request #13547 from meeseeksmachine/auto-backport-of-pr-93…
ff98f1c 
…14-on-v3.1.x

Backport PR #9314 on branch v3.1.x (Simplify units.Registry.get_converter.)
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

jklymak jklymak approved these changes
dstansby dstansby left review comments
timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

topic: units and array ducktypes

Projects

None yet

Milestone

v3.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL