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

PSet.intersect #99

Merged
merged 3 commits into from
Sep 21, 2022
Merged

PSet.intersect #99

merged 3 commits into from
Sep 21, 2022

Conversation

prdoyle
Copy link
Contributor

@prdoyle prdoyle commented Sep 17, 2022

Fixes #96.

Unit tests

I've included some JUnit5 @ParameterizedTests. Apologies that the naming scheme for the test methods is so different. The existing JUnit4 scheme won't work because JUnit4 will think they're JUnit4 tests and they won't work. Since JUnit5 needs a different convention anyway, I've used the one I'm accustomed to, which takes the form conditions_expectedResult.

@hrldcpr
Copy link
Owner

hrldcpr commented Sep 19, 2022

Awesome! I'll test this out and merge it soon

@hrldcpr hrldcpr merged commit 16ebf18 into hrldcpr:master Sep 21, 2022
@hrldcpr
Copy link
Owner

hrldcpr commented Sep 21, 2022

This is great, thanks! The tests seem nice and thorough. I think at some point we should optimize it by having it check if list is a Set, and then if so preferring to iterate over whichever set is smaller (so we get O(mlgn) with m<n) …but anyway that can be a future thing, I'll open an issue as a reminder.

@hrldcpr
Copy link
Owner

hrldcpr commented Sep 21, 2022

#103

@prdoyle
Copy link
Contributor Author

prdoyle commented Sep 21, 2022

@hrldcpr - I shied away from iterating over the shorter set because I wasn't sure I could preserve order correctly for the ordered set. The implementation I provided gives, I think, a logical ordering in all cases. (At least, it was the ordering that met the needs of my application. 😅)

I probably could have added special cases if a or b is empty or singleton.

For the implementation a.minusAll(a.minusAll(b)), I think it always takes |a|+|b|-|a∩b| steps, which is ok but not awesome. Certainly some subclasses could do much better.

Another implementation could be based on looping through a and calling minus for each element that returns false for b.contains. That would be O(a log b), which I think is never better unless |b| <= 2.

@hrldcpr
Copy link
Owner

hrldcpr commented Sep 22, 2022

Good point about maintaining order. And yeah the different possible complexities are intriguing/subtle!

I like the current implementation, but I'll keep #103 open in case anyone stumbles upon it with a brilliant insight.

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.

PSet has no set intersection operation
2 participants