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

Merge upstream? StableSet: Adding an additional implementation, new features and tests and more. #91

Open
idanmiara opened this issue Feb 13, 2023 · 6 comments · May be fixed by #92
Open

Comments

@idanmiara
Copy link

idanmiara commented Feb 13, 2023

Hi @rspeer
Thanks for this nice library!

I made a very large refactor, added another implementation, many features, tests and compatibility with other implementations and some more. The new version passes all the tests from the other 3 repositories almost completely unchanged.
https://github.com/idanmiara/ordered-set

Since that's a major addition, I didn't know how/if you would want to merge this work and since I needed to use this for some project already - I released it under to pypi under a new name stableset (with proper credits).
Having said that, I'd be happy to merge this work upstream if you are interested, if so, let me know how you'd like to proceed with this.

@twiddli
Copy link

twiddli commented Feb 13, 2023

Hi, would you mind supporting this too #79?

This was referenced Feb 13, 2023
@NeilGirdhar
Copy link

NeilGirdhar commented Apr 4, 2023

The refactor's elimination of the list is a significant improvement. The ordering preserved by the interface for intersection is also an improvement.

FYI, You can make intersection a lot faster (linearithmic in the smallest set rather than linear in the first set) if you implement it like this: https://discuss.python.org/t/add-orderedset-to-stdlib/12730/83?u=neilgirdhar. This comes at the cost of storing some integers, which may or may not be desirable.

I also don't see the point of add returning the length of the set. It's neither in the ABC nor is it particularly helpful.

@NeilGirdhar
Copy link

@twiddli pretty sure he can't support it since dicts don't have efficient indexing.

@idanmiara
Copy link
Author

@twiddli @NeilGirdhar
Thanks for your inputs, I'll look into it in about 2 weeks when I'm back from a holiday.

@jagerber48
Copy link

@idanmiara a few questions.

  1. "Both have similar interfaces but differ in respect of their implementation and performance." I'm curious to know a listing of exactly how the interfaces differ if at all.

  2. Why the name StableSet? And why do you call the dict-based set StableSet and the list-based set OrderedSet? Is the choice arbitrary? If the interfaces are the exact same (except for performance trade-offs or how equality is calculated) maybe they could all share a name (OrderedSet would be my suggestion) and the "style" could be selected with an input option during instantiation?

  3. Have you seen this thread? I wonder if anything there is helpful.

@idanmiara
Copy link
Author

@jagerber48 thanks, I'll look into it when I'm back from my holiday.

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 a pull request may close this issue.

4 participants