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

Comparators support partial ordering #5778

Merged
merged 18 commits into from
Mar 7, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Feb 28, 2023

All ordered and unordered comparators support partial ordering, not only total ordering - by returning Nothing from some methods.

  • Refactor Incomparable_Values to a builtin error

Bench results

  • Equality.enso:
    • Speedup in Integer, Boolean, and Text
  • EqualsBenchmarks:
    • equalsPrimitives speedup by 20%
    • equalsStrings speedup by 21%

@Akirathan Akirathan linked an issue Feb 28, 2023 that may be closed by this pull request
@Akirathan Akirathan self-assigned this Mar 1, 2023
@Akirathan Akirathan force-pushed the wip/akirathan/partial-ord-comparator-5771 branch from b5b94d6 to c927c6e Compare March 2, 2023 14:23
@Akirathan Akirathan mentioned this pull request Mar 2, 2023
5 tasks
@Akirathan Akirathan marked this pull request as ready for review March 6, 2023 07:49
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

#5771 suggests to merge compare and equals, get rid of is_ordered and always just call compare. I still think it should be done... at least one day.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good, just some suggestions, mostly I think it would be good to slightly expand on a few test cases.

  1. Additionally, I'm wondering how the (Hash)Map behaves in presence of incomparable values - I'm not exactly sure now - so I think it would be beneficial to add some tests showing what is the expected behaviour and that we are conscious of it.

My suggestion:

m = Map.from_vector allow_duplicates=True [[Number.nan, 1], [Number.nan, 2], [Number.nan, 3], [0.5, 4]]
m.size == ??? # idk, are the nans merged or not?
m.contains_key Number.nan == ??? # it does contain it but due to incomparable I guess it may not say that?
m.get Number.nan == ??? # as above
m.get 0.5 . should_equal 4

NaN handling to be addressed in a separate issue #5833

  1. I think we should add a test for 2 arrays/hashmaps which contain incomparable values and verify what is the result.

For example

a = [Number.nan, 2]
b = [Number.nan, 2]
(a == b) == ???

I guess it is False but let's have a test showing that.

Also - do you think it is correct for this to be False? My initial intuition would be that the incomparability should be 'poisonous' and if any field in an Atom/Array/Map is incomparable, the whole thing should be incomparble as well. Although I remember the discussions about equality some longer time ago where this was not that obvious because it has its own problems. @JaroslavTulach what do you think about this case?

My_Atom.Value Number.nan == My_Atom.Value Number.nan

should this be False or throw Incomparable_Values?

Personally, I'd go for throwing incomparable values, but I'm not attached to this opinion if anyone thinks False is somehow better (although will appreciate some arguments as to why).

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Mar 6, 2023
@mergify mergify bot merged commit b6e2319 into develop Mar 7, 2023
@mergify mergify bot deleted the wip/akirathan/partial-ord-comparator-5771 branch March 7, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for partial ordering to Comparators
3 participants