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

Implement a method similar to the Python set.pop() #18652

Open
bmcdonald3 opened this issue Oct 28, 2021 · 5 comments
Open

Implement a method similar to the Python set.pop() #18652

bmcdonald3 opened this issue Oct 28, 2021 · 5 comments

Comments

@bmcdonald3
Copy link
Member

Python supports a set.pop() method which removes and returns a random element from the set. This could be valuable when trying to use your set as an unordered queue and other cases.

Python pop: https://www.w3schools.com/python/ref_set_pop.asp

@bradcray
Copy link
Member

I'm good with this functionality, but am not a fan of pop() as a name for this because it implies to me more ordering than our sets have and makes me think there should be a push() as well. So my head goes to things like removeArbitraryElement() (wordy), removeArbitraryElt(), removeArbitrary(), removeSomething(), removeElement() or the like.

In the meeting today, I proposed an argument-less .remove() ("remove something, I'm not going to say what"), but others pointed out that this might be interpreted as "removing the set itself" in some way, as well as being subtle, which I agree with.

I couldn't find an equivalent of pop()/removeArbitrary()/removeElement() in Rust, which surprised me slightly. But they do have a drain() method which seems to be an iterator that removes elements and yields them. So if we thought that the main use of pop() was a work-queue type of thing, we could potentially take that approach instead (though I don't think it would support the pattern Andrew was talking about in which we might simultaneously be adding things back into the set as we drained it... or at least, that feels scary / potentially expensive to me, at least in a parallel setting). Why do I mention this? In part because the name drain() seems natural / intuitive here, and I don't currently have an alternative to pop() that I feel particularly strong about.

@lydia-duncan
Copy link
Member

I'm okay with drain but would throw something like removeRandomElt into the ring as an option (or removeRandom)

@bmcdonald3
Copy link
Member Author

a drain() method which seems to be an iterator that removes elements and yields them

I don't think I like this approach since, as you mention, it doesn't apply to the only real use-case that we were able to come up with in the original discussion here, so it feels almost like adding something without a very compelling reason to do so, which makes me think this may be a better after 2.0 effort.

Returning to the original question of adding Python's pop() like functionality here, I would be happy with the name drain(), though, I am now less certain that this should happen pre-2.0 since the only real use case we were able to come up with was purely hypothetical and IIRC, the only argument for including this in the module review was to match the Python implementation more fully.

Do either @lydia-duncan or @bradcray have an opinion on if this should be a pre- or post-2.0 effort?

@lydia-duncan
Copy link
Member

I was viewing it as a post-2.0 effort, yeah

@bradcray
Copy link
Member

bradcray commented Dec 7, 2021

It would not surprise me if someone will end up wanting it prior to 2.0 (in which case we should add it), but I agree that we don't need to solve it in order to declare 2.0.

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

No branches or pull requests

3 participants