-
Notifications
You must be signed in to change notification settings - Fork 2
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
[ new ] Map and Set #3
Conversation
Thanks for this! One question: How does this perform compared with |
I haven't thoroughly tested yet, but will certainly do so, and report back here. |
…ow interface for Map k v.
…nnecessary Show implementation for Map k v.
…d fixing size function.
This should be ready for you to look at when you have a chance. |
…n, and difference.
src/Data/Map.idr
Outdated
export | ||
find : Ord k => k -> Map k v -> v | ||
find _ Tip = assert_total $ idris_crash "Map.!: given key is not an element in the map" | ||
find k (Bin _ kx x l r) = | ||
case compare k kx of | ||
LT => find k l | ||
GT => find k r | ||
EQ => x |
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.
This is not a total function, because it can crash at runtime when the key is not in the map. We must therefore not use assert_total
here: assert_total
is for those cases where we know (and can proof) that a function is total but have no other way to convince Idris. This function should be partial
.
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.
Awesome catch here, I totally forgot to add those annotations, they should be added now via 5586d97.
This is incredible and really, really cool. The API is huge, and it's clean code and well documented. Thank you so much for doing this. I did not yet have the time to look at the whol thing in detail, (it's a lot of code), but I'm deeply impressed by the thoroughness of it. We should have some tests for this, but I suggest the following: We should soon merge this as it is, and then we can set up some property tests in a later PR. That way, CI will run automatically, without me always clicking the "approve" button. I can setup some basic Hedgehog tests if you haven't used those before, but for some of the more intricate functions it will make sense if you come up with some test cases of your own. Again, this is huge, so thank you very much for your contribution! |
You're welcome, really glad I could help contribute this! I'm very excited by the prospect of having these data structures at the communities' disposal now. That sounds great, this should be in a good place to merge as of now (barring any linting issues). I really like the idea of setting up a CI pipeline, and I would definitely be interested in helping write some of the more nuanced tests. |
Linting issues should be fixed via 953a4ce and good to merge. |
Wanted to check in and see if there was anything else needed before merging this PR. |
Sorry for the delay. And once again: Thanks for adding this. It looks really well designed and documented. |
No worries at all! Very excited for this PR. |
This PR adds a finite
Map
andSet
implementation, ported from Haskell.Map
This
Map
implementation is very much on par (if not a bit more performant) withSortedMap
in terms of performance.Map
is faster thanSortedMap
for the following operations:fromList
(most likely stemming from a preliminary sorting test prior to generating the internal tree)delete
update
lookup
keys
values
Map
is slower thanSortedMap
for the following operation:insert
Set
This
Set
implementation is very much on par (if not a bit more performant) withSortedSet
in terms of performance.Set
is faster thanSortedSet
for the following operations:fromList
(most likely stemming from a preliminary sorting test prior to generating the internal tree)delete
member
(contains
)union
intersection
difference
Set
is slower thanSortedSet
for the following operation:insert
Summary
Overall, I think this PR offers solid implementations of both a map/dictionary and set data structure which both can provide great utility covering numerous use cases.
Results (Profiling)