Skip to content
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

Add select_keys. #393

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add select_keys. #393

wants to merge 9 commits into from

Conversation

eigenhombre
Copy link
Member

@eigenhombre eigenhombre commented Mar 30, 2018

Fixes #392.

Copy link
Member

@mrocklin mrocklin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome back @eigenhombre ! It's good to see you.

Since you've been away much has happened. In general this project has become much more conservative about adding new functions, mostly because we were flooded with many PRs for specialty functions that would have made the namespace harder to navigate. I genuinely don't know what our policy is these days on what to accept and what not to accept. @eriknw may be more up to date than I am.

Another constraint we've considered adding is asking people to submit Cython solutions to CyToolz, though I don't think that this has ever been enforced.

{}
"""
rv = factory()
for k in set(keys).intersection(d.keys()):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend spelling this out explicitly for performance reasons:

for k in keys:
    if k in d:
        rv[k] = d[k]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@eigenhombre eigenhombre changed the title Fixes #392. Add select_keys. Apr 3, 2018
@eigenhombre
Copy link
Member Author

@mrocklin Nice to be back. I think select_keys is a good addition, and in my experience from Clojure it is no more "specialized" than assoc or update, but please let me know if the consensus is otherwise.

@mrocklin
Copy link
Member

mrocklin commented Apr 4, 2018

Lets see if @eriknw has any thoughts in the next day or so and if not, then merge.

@mrocklin
Copy link
Member

mrocklin commented Apr 4, 2018

Should also remember to add this to the API documentation

@eriknw
Copy link
Member

eriknw commented Apr 5, 2018

Indeed, welcome back @eigenhombre!

Sorry for my absence. I'm (mostly) off-the-grid this week dealing with a family event.

select_keys seems like a reasonable addition. I like that it's shorter than other ways to spell it:

In [1]: {k: v for k, v in d.items() if k in keys}
In [2]: {k: d[k] for k in keys if k in d}
In [3]: keyfilter(keys.__contains__, d)
In [4]: select_keys(d, keys)

I'm not sure about the argument order. It seems to me that select_keys(keys, d) would curry better. How often have we deviated from function signatures in Clojure?

We don't require that contributors add to CyToolz as well, but it is appreciated.

Okay, gotta go!

@eigenhombre
Copy link
Member Author

Hi again! @eriknw , in Clojure's assoc, update, update-in, get-in, select-keys, etc. the dictionary ("map") argument is first; conversely the collection argument to collection-centric functions are last (map, filter, partition, sort-by, etc.). All the equivalent dictionary operations that I saw in dicttoolz have the dictionary first, so that'd be my vote.

Will take a look at the API docs to refresh my memory about how that's done and whether I can add that to the PR....

@mrocklin
Copy link
Member

mrocklin commented Apr 5, 2018

Having the keys first would allow for nice currying and use of Pipe, which has become somewhat idiomatic in the use of toolz

from toolz.curried import pipe, select_keys, map, filter, get, frequencies

pipe(data, map(select_keys(['name', 'age'])), filter(lambda d: d['age'] > 10), get('name'), frequencies)

Clojure has more options here, so this isn't as big of a deal for them

@eigenhombre
Copy link
Member Author

@mrocklin @eriknw Thanks for the suggestions. Added select_keys to docs (is more needed there?). Swapped argument order (not without slight reservation, as it deviates from assoc and friends). Let me know if either of you has other suggestions!

@eigenhombre
Copy link
Member Author

Shall I close this @mrocklin @eriknw ?

@chbrown
Copy link

chbrown commented Dec 10, 2018

👍 I too would like to see this in the toolz API.

IMO select_keys argument order should be select_keys(d: dict, keys: list) -> dict, following Clojure.
I'd bet Rich has thought about this more than any of us :)

@epgui
Copy link

epgui commented Jul 2, 2022

I don't understand why this was closed.

I came looking for select_keys in this library, found this closed PR, read through the comments... It seems like there was some alignment in favour of adding the function, no? 🤔

@eigenhombre
Copy link
Member Author

eigenhombre commented Jul 3, 2022

@epgui : I closed it b/c I'd rather not merge unilaterally without one of @mrocklin / @eriknw +1'ing, since they have put more work into this project than I have.

I also would prefer the argument order that Clojure uses.

IMO this library has gone in a somewhat different direction than what I would prefer: my preference would be to provide a fairly simple set of functions which are (C)Python drop-ins for the highly cohesive set of collections operations provided by Clojure. I haven't done a broad survey of the available libraries to know if there is a good fit out there, or if another library is called for. Am interested in what other views in the community might be.

@eigenhombre
Copy link
Member Author

Reopening to encourage further discussion, since clearly there has been some demand for this.

@eigenhombre eigenhombre reopened this Jul 3, 2022
@epgui
Copy link

epgui commented Jul 4, 2022

@eigenhombre

my preference would be to provide a fairly simple set of functions which are (C)Python drop-ins for the highly cohesive set of collections operations provided by Clojure

Strongly agreed, but I don't even think it has to stop there. A big part of why FP is so great, IMO, is that it allows for a richer "vocabulary" of functions, which provides for more expressive and concise code (alternatively, the same idea but in the words of the pytoolz developers). I think we could easily argue for inclusion of functions from Haskell (eg.: much of what you'd find in Prelude), or even what you'd find in other FP libraries (eg.: lodash, ramda.js, etc). Having a long list of functions available doesn't complicate anything for the user, but it does make organizing docs more important.

I don't have the strongest opinion on the order of arguments, on the other hand. Clojure's philosophy of having "values" come first and "lists of values" come last works very well with thread-first and thread-last macros, but I also understand the other opinion expressed in this thread. I think you could reasonably argue for either way, and I think the best decision would be the most self-consistent decision (and not necessarily the one that is most consistent with other languages or libraries).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select-keys equivalent
5 participants