-
-
Notifications
You must be signed in to change notification settings - Fork 11k
ENH: Add support for dict view objects. #14539
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PyObject *mod = PyImport_ImportModule("collections.abc"); | ||
|
||
if (mod != NULL) { | ||
mappingview = PyObject_GetAttrString(mod, "MappingView"); |
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.
If we're going to do this, we should use Collection
Some remarks:
|
I think that was the reason why MappingView, because there is a PEP fairly recently, that I believe guarantees stable ordering. I agree that the better thing would be python registering with sequence (I think there was some reason why not, but cannot think of it right now). To be clear, I am not sure I like it much, but it is apparently pretty annoying for downstream such as matplotlib. EDIT: Hmmm, I guess the closest PEP is likely the one for stable order of kwargs. I guess it is true that this is stable, but not sure it is spelled out quite clearly anywhere. |
Because they don't implement I think I'd be in favor of deprecating |
I deprecating any "fallthrough" to object is probably a good plan (and then rudder back on specifics probably ;)), the reason I had looked it at all was the argument that it is at least ordered and that matplotlib says it is pretty painful for them. |
Is there a matplotlib issue somewhere cataloging that pain? I ask because most py2->py3 conversion would replace |
ping @story645. Might be pinging the wrong person, but do you want to chime in Hannah on why matpltolib would like the dictview inside arrays? |
It's more that plotting dictionary keys and views is fairly common use case because that's how a lot of frequency data is stored & that list(dict.values()) is painful in tutorials & example docs. matplotlib/matplotlib#6787 was merged to patch this (since modified a drop) but the discussion in those comments was that the fix should really go upstream 'cause it's working around Numpy's limitations. |
It seems like the argument is roughly "Why should I have to call The problem is, s = lambda: {1, 2, 3}
d = lambda: dict(a=1, b=2)
it = lambda: (i for i in range(2))
to_test = (s, d, lambda: d().keys(), lambda: d().values(), lambda: d().items(), it)
for val in to_test:
print("array( ... ): {!r}".format(np.array(val())))
print("array(list(...)): {!r}".format(np.array(list(val()))))
print()
It seems strange to me to change the behavior of some of these without changing all of them. I'm not leaning particularly in one way or the other on whether making the user call
|
I am in favor of making changes because |
@eric-wieser Would it be a good idea to just add a list() wrapper to the input? That would guarantee consistency and avoid any weird cases |
In a recent community call we talked about this and decided not to support this use case. If Python ends up accepting a change to make dict.values() a sequence, we would automatically support it, but we do not want to do that change on our own. |
fixes #573
accepting dict views come from: seberg@9809763