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

New functions: fold_on_nonequal_inter and fold_on_nonequal_union #6

Merged
merged 14 commits into from
May 10, 2024

Conversation

mlemerre
Copy link
Contributor

@mlemerre mlemerre commented May 7, 2024

Was called fold_on_diff in Codex. No tests yet

@mlemerre mlemerre requested a review from dlesbre May 7, 2024 22:59
@mlemerre mlemerre added this to the 0.10.0 milestone May 7, 2024
Copy link
Collaborator

@dlesbre dlesbre left a comment

Choose a reason for hiding this comment

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

I've suggested some corrections on improvements.

It would be nice, and fairly easy, to have a specialized version of these functions for non-generic maps.

Also adding tests shouldn't be too hard.

patriciaTree.ml Outdated Show resolved Hide resolved
patriciaTree.ml Outdated Show resolved Hide resolved
patriciaTree.ml Outdated Show resolved Hide resolved
@dlesbre dlesbre added the enhancement New feature or request label May 8, 2024
@mlemerre
Copy link
Contributor Author

mlemerre commented May 8, 2024

Thanks for the suggestion! I forgot to tell that the patch was not ready yet (homogeneous version and tests). I was not fully aware of the unsigned changes, so thanks for the improvements ideas

@mlemerre mlemerre force-pushed the feature/fold_on_differences branch from 22b6cc8 to b6f860c Compare May 8, 2024 20:06
@mlemerre
Copy link
Contributor Author

mlemerre commented May 9, 2024

It should be good now: I have added the tests and the homogeneous version. The tests obviously revealed that I messed up with the union case (much more complicated than the intersection one).

If you find a way to improve the code, then go for it (I don't see obvious ways to do it). Also please tell me if you think the doc is fine. Then we should be able to merge!

Copy link
Collaborator

@dlesbre dlesbre left a comment

Choose a reason for hiding this comment

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

I've replaced a remaining ma < mb with unsigned_lt, simplified g by removing extra arguments, and improved the doc a bit (multiline code blocks with [ ... ] don't render well: they keep all whitespace as is).

@mlemerre mlemerre merged commit bef2736 into main May 10, 2024
3 checks passed
@dlesbre dlesbre deleted the feature/fold_on_differences branch May 11, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants