Simplify units.Registry.get_converter.#9314
Conversation
dstansby
left a comment
dstansby
left a comment
There was a problem hiding this comment.
A couple of things that I think need leaving in.
| def get_converter(self, x): | ||
| 'get the converter interface instance for x, or None' | ||
|
|
||
| if not len(self): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| converter = None | ||
| classx = getattr(x, '__class__', None) | ||
|
|
||
| if classx is not None: |
There was a problem hiding this comment.
Shouldn't this also stay in, as there's no guarantee that unit data is a subclass of numpy array or an iterable?
There was a problem hiding this comment.
Should be handled by return self[type(x)], no?
|
(but I think simplifying the numpy logic is a good thing to do!) |
anntzer
force-pushed
the
simpler-units
branch
from
7308486 to
d5adae3
Compare
|
Please hold of on merging this for 2.2. |
|
I think this should wait for the units MEP, if such is forthcoming... |
|
AFAICT this should not change any semantics. |
|
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
force-pushed
the
simpler-units
branch
from
eff243f to
a611314
Compare
|
rebased |
anntzer
force-pushed
the
simpler-units
branch
from
a611314 to
6a71555
Compare
|
... good thing we added some tests 😉 |
anntzer
force-pushed
the
simpler-units
branch
from
6a71555 to
ece3282
Compare
|
good catches, should be fixed now. |
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
This looks right to me. Could have an extra comment or two, for instance after the except KeyError, but I supposes its clear enough
|
|
||
| # 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"): |
There was a problem hiding this comment.
@ImportanceOfBeingErnest had some concerns about this way to check for pandas:
#11664 (comment)
There was a problem hiding this comment.
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.
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
force-pushed
the
simpler-units
branch
from
ece3282 to
25cae8e
Compare
|
added comments |
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
This looks right, but I would be more comfortable if there were some tests.
|
There are already tests (as noted by Jody anbove, an earlier version of this failed them). |
|
There are some tests. I'd still argue there are not enough, but I guess we will find out what breaks... |
…14-on-v3.1.x Backport PR #9314 on branch v3.1.x (Simplify units.Registry.get_converter.)
5 participants
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