Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Simplify units.Registry.get_converter.#9314
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dstansby left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A couple of things that I think need leaving in.
| defget_converter(self,x): | ||
| 'get the converter interface instance for x, or None' | ||
| ifnotlen(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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) | ||
| ifclassxisnotNone: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be handled byreturn self[type(x)], no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah yes
dstansby commentedFeb 10, 2018
(but I think simplifying the numpy logic is a good thing to do!) |
tacaswell commentedFeb 10, 2018
Please hold of on merging this for 2.2. |
jklymak commentedJul 9, 2018
I think this should wait for the units MEP, if such is forthcoming... |
anntzer commentedJul 10, 2018
AFAICT this should not change any semantics. |
jklymak commentedFeb 27, 2019
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 commentedFeb 27, 2019
rebased |
jklymak commentedFeb 27, 2019
... good thing we added some tests 😉 |
anntzer commentedFeb 27, 2019
good catches, should be fixed now. |
jklymak left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.""" | ||
| ifhasattr(x,"values"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ImportanceOfBeingErnest had some concerns about this way to check for pandas:
#11664 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One could do something liketype(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 workas the correct converter for ndarrays depends on the type of thecontents, 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 underlyingdata, even if it is masked (as it should have the same type as theunmasked entries), for the purpose of getting the converter.
anntzer commentedFeb 28, 2019
added comments |
timhoffm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks right, but I would be more comfortable if there were some tests.
anntzer commentedFeb 28, 2019
There are already tests (as noted by Jody anbove, an earlier version of this failed them). |
jklymak commentedFeb 28, 2019
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.xBackport PR#9314 on branch v3.1.x (Simplify units.Registry.get_converter.)
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