-
Notifications
You must be signed in to change notification settings - Fork 323
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
NaN is a new builtin type #5744
Conversation
3ae8afe
to
489ce95
Compare
Comparable.from (that:Number) = | ||
case that.is_nan of | ||
True -> Incomparable | ||
False -> Default_Ordered_Comparator |
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 used to be a huge performance bottleneck.
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.
Is there some speedup after the rewrite?
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.
Is there some speedup after the rewrite?
Yes. sieve.enso
is 2x faster, and EqualsBenchmarks.equalsPrimitives
is 10% faster. I have not tried other benchmarks, but I expect similar results, certainly no regressions.
Comparable.from (that:Number) = | ||
case that.is_nan of | ||
True -> Incomparable | ||
False -> Default_Ordered_Comparator |
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.
Is there some speedup after the rewrite?
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/TypeOfNode.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/type/TypesFromProxy.java
Show resolved
Hide resolved
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.
Can you add a test for when NaN is somewhere in a Vector and you try to sort it?
That was a requirement by @jdunkerley IIRC
This will be tackled in #5742, in a follow-up PR. |
3275867
to
d7e715d
Compare
engine/runtime/src/test/java/org/enso/interpreter/test/MetaObjectTest.java
Show resolved
Hide resolved
# Conflicts: # CHANGELOG.md
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.
Code looks good.
I'm hesitant of expanding our numeric type hierarchy like that, it feels like it will drive up the complexity of the type system and anything that relies on that hierarchy.
But it seems to work for what we need right now, so not arguing against it.
+1 - Do Not Expose Deep Hierarchies, Practical API Design page 83
Do you mean implementation? No, that's OK. I should probably note that the core flaw is our "attempt to not support partial order on any type". If our comparator method could return:
we wouldn't have this problem. I am OK with the introduction of Then we wouldn't need to create an artificial type like |
Let's stick with the current design for now and see where it takes us. I assume that we will know for sure during the work on #5626. Potential rollback to |
Introduce new
Nan
builtin type that hasDecimal
as its super type.