Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
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.
| 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.
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) | ||
| if classx is not None: |
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
(but I think simplifying the numpy logic is a good thing to do!) |
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... |
rebased |
... good thing we added some tests 😉 |
good catches, should be fixed now. |
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.""" | ||
| if hasattr(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.
added comments |
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.
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.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