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

Add Warnings whenever a floating-point value is used as a Dictionary key #11417

Merged
merged 24 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f1f4033
attach warning
GregoryTravis Oct 25, 2024
835dba1
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Oct 29, 2024
1c4fd38
insert no_warning, make distinct tests consistent
GregoryTravis Oct 29, 2024
3f2847e
rename make_random_vec
GregoryTravis Oct 29, 2024
2277fa5
merge
GregoryTravis Oct 30, 2024
bb9affb
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Oct 31, 2024
f9488c0
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 1, 2024
84dbcba
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 4, 2024
90fe26b
from_vector test
GregoryTravis Nov 4, 2024
afead1a
add sparse
GregoryTravis Nov 4, 2024
78d19ab
sparse
GregoryTravis Nov 4, 2024
2aefd64
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 5, 2024
5cb0fc2
case instead of is_a
GregoryTravis Nov 5, 2024
8894202
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 6, 2024
682381f
combine error with FPE
GregoryTravis Nov 6, 2024
a900b4f
wip
GregoryTravis Nov 6, 2024
61a4e1f
doc warning
GregoryTravis Nov 6, 2024
6a00aca
review
GregoryTravis Nov 6, 2024
cca8aae
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 8, 2024
5926e02
merge
GregoryTravis Nov 12, 2024
63eadb4
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 13, 2024
50691be
redundant import
GregoryTravis Nov 13, 2024
f665b51
Merge branch 'develop' into wip/gmt/9608-warn-float-dict-key
GregoryTravis Nov 13, 2024
0e3fa03
bump
GregoryTravis Nov 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import project.Any.Any
import project.Data.Numbers.Float
import project.Data.Numbers.Integer
import project.Data.Pair.Pair
import project.Data.Text.Text
Expand All @@ -11,8 +12,10 @@ import project.Meta
import project.Metadata.Widget
import project.Nothing.Nothing
import project.Panic.Panic
import project.Warning.Warning
from project.Data.Boolean import Boolean, False, True
from project.Data.Text.Extensions import all
from project.Errors.Common import Float_Used_As_Dictionary_Key
from project.Metadata.Choice import Option
from project.Metadata.Widget import Single_Choice, Vector_Editor
from project.Widget_Helpers import make_all_selector
Expand Down Expand Up @@ -163,9 +166,14 @@ type Dictionary key value
example_insert = Examples.dictionary.insert 7 "seven"
@key (make_all_selector ..Always)
@value (make_all_selector ..Always)
insert : Any -> Any -> Dictionary
insert self key=(Missing_Argument.throw "key") value=(Missing_Argument.throw "value") =
self.insert_builtin key value
insert : Any -> Any -> Boolean -> Dictionary
insert self key=(Missing_Argument.throw "key") value=(Missing_Argument.throw "value") no_warning:Boolean=False =
new_dict = self.insert_builtin key value
case key of
_ : Float ->
if no_warning then new_dict else
Warning.attach (Float_Used_As_Dictionary_Key.Warning key) new_dict
_ -> new_dict

## GROUP Selections
ICON table_clean
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import project.Data.Numbers.Float
import project.Data.Numbers.Integer
import project.Data.Text.Extensions
import project.Data.Text.Text
Expand Down Expand Up @@ -552,7 +553,7 @@ type Out_Of_Range
## PRIVATE
to_text : Text
to_text self =
extra = if self.message.is_nothing then "" else ": "+self.message.to_text
extra = if self.message.is_nothing then "" else " (message = "+self.message.to_text+")"
"(Out_Of_Range (value = "+self.value.to_text+")" + extra + ")"

## Indiciates that the response from a remote endpoint is over the size limit.
Expand All @@ -576,3 +577,19 @@ type Response_Too_Large
Convert the Java exception to an Enso dataflow error.
handle_java_exception ~action =
Panic.catch ResponseTooLargeException action (cause-> Error.throw (Response_Too_Large.Error cause.payload.getLimit))

## Indicates that a Float was used as a Dicionary key.
type Float_Used_As_Dictionary_Key
Copy link
Member

Choose a reason for hiding this comment

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

This type feels slightly redundant with already existing Floating_Point_Equality.

However, I do like that we get a more precise message ('Float used as dictionary key'). What about merging this into the Floating_Point_Equality error, perhaps by distinguishing different constructors (we already have multi-constructor errors, e.g. Invalid_Value_Type).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done -- moved Floating_Point_Equality into Base.

## PRIVATE
Warning value:Float

## PRIVATE
Create a human-readable version of the error.
to_display_text : Text
to_display_text self =
"Float used as dictionary key: "+self.value.to_text

## PRIVATE
to_text : Text
to_text self =
"(Float_Used_As_Dictionary_Key (value = "+self.value.to_text+"))"
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ distinct vector on =
key = on item
if (existing.get key False) then existing else
builder.append item
existing.insert key True
existing.insert key True no_warning=True

duplicates vector on = Vector.build builder->
counts = vector.fold Dictionary.empty current-> item->
Expand Down
11 changes: 11 additions & 0 deletions test/Base_Tests/src/Data/Dictionary_Spec.enso
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from Standard.Base import all
import Standard.Base.Errors.Illegal_Argument.Illegal_Argument
import Standard.Base.Errors.No_Such_Key.No_Such_Key
from Standard.Base.Errors.Common import Float_Used_As_Dictionary_Key

from Standard.Test import all

Expand Down Expand Up @@ -269,6 +270,16 @@ add_specs suite_builder =
m.at 200 . should_equal 3
m.at Nothing . should_equal 1

group_builder.specify "should attach a warning when a Float is used as a key" <|
m = Dictionary.empty . insert 1.2 3
m.at 1.2 . should_equal 3
Problems.expect_only_warning Float_Used_As_Dictionary_Key m
Copy link
Member

Choose a reason for hiding this comment

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

Please also add a test that checks if we have the same behaviour when we construct the dictionary using Dictionary.from_vector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


group_builder.specify "should attach a warning when a Float is used as a key (using from_vector)" <|
m = Dictionary.from_vector [[2, 3], [1.2, 3], [4, 5]]
m.at 1.2 . should_equal 3
Problems.expect_only_warning Float_Used_As_Dictionary_Key m

suite_builder.group "Polyglot keys and values" group_builder->
group_builder.specify "should support polyglot keys" pending=pending_js_missing <|
dict = Dictionary.singleton (js_str "A") 42
Expand Down
25 changes: 18 additions & 7 deletions test/Benchmarks/src/Map/Hash_Map.enso
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ polyglot java import org.enso.benchmark_helpers.JavaHashMapWrapper
options = Bench.options . set_warmup (Bench.phase_conf 2 2) . set_measure (Bench.phase_conf 2 3)

type Data
Value ~ints
Value ~dense_ints ~sparse_ints

create n =
create_ints =
create_dense_ints =
Vector.new n _->
Random.integer 0 ((n.div 100) - 1)
Data.Value create_ints
create_sparse_ints =
Vector.new n _->
Random.integer 0 (n - 1)
Data.Value create_dense_ints create_sparse_ints

type Scenario
Instance map_constructor
Expand Down Expand Up @@ -43,15 +46,23 @@ collect_benches = Bench.build builder->

builder.group ("Enso_Hash_Map_" + n.to_text) options group_builder->
# Scenario similar to what is done in distinct
# 'Sparse' ints have fewer duplicates and cause more inserts to happen.
group_builder.specify "Enso_Sparse_Incremental" <|
Scenario.Instance (_ -> Dictionary.empty) . run_distinct data.sparse_ints
group_builder.specify "Java_Sparse_Incremental" <|
Scenario.Instance (_ -> JavaHashMapWrapper.new) . run_distinct data.sparse_ints

# Scenario similar to what is done in distinct
# 'Dense' ints have more duplicates and cause fewer inserts to happen.
Copy link
Member

Choose a reason for hiding this comment

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

I'd add

Suggested change
# Scenario similar to what is done in distinct
# 'Dense' ints have more duplicates and cause fewer inserts to happen.
# Scenario similar to what is done in distinct
# 'Dense' ints have more duplicates and cause fewer inserts, but more key updates to happen.

(assuming it's true, but it feels it should be)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

group_builder.specify "Enso_Incremental" <|
Scenario.Instance (_ -> Dictionary.empty) . run_distinct data.ints
Scenario.Instance (_ -> Dictionary.empty) . run_distinct data.dense_ints
group_builder.specify "Java_Incremental" <|
Scenario.Instance (_ -> JavaHashMapWrapper.new) . run_distinct data.ints
Scenario.Instance (_ -> JavaHashMapWrapper.new) . run_distinct data.dense_ints

# A scenario similar to what is done in add_row_number with grouping
group_builder.specify "Enso_Replacement" <|
Scenario.Instance (_ -> Dictionary.empty) . run_count_keys data.ints
Scenario.Instance (_ -> Dictionary.empty) . run_count_keys data.dense_ints
group_builder.specify "Java_Replacement" <|
Scenario.Instance (_ -> JavaHashMapWrapper.new) . run_count_keys data.ints
Scenario.Instance (_ -> JavaHashMapWrapper.new) . run_count_keys data.dense_ints

main = collect_benches . run_main
26 changes: 18 additions & 8 deletions test/Benchmarks/src/Vector/Distinct.enso
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,31 @@ options = Bench.options . set_warmup (Bench.phase_conf 1 3) . set_measure (Bench


type Data
Value ~random_vec ~uniform_vec ~random_text_vec ~uniform_text_vec
Value ~random_integer_vec ~uniform_integer_vec ~random_float_vec ~uniform_float_vec ~random_text_vec ~uniform_text_vec

create =
Data.Value create_random_vec create_uniform_vec create_random_text_vec create_uniform_text_vec
Data.Value create_random_integer_vec create_uniform_integer_vec create_random_float_vec create_uniform_float_vec create_random_text_vec create_uniform_text_vec


create_random_vec = Utils.make_random_vec 100000
create_random_integer_vec = Utils.make_random_integer_vec 100000


create_uniform_vec = Vector.fill 100000 1
create_random_float_vec = Utils.make_random_float_vec 100000


create_uniform_integer_vec = Vector.fill 100000 1


create_uniform_float_vec = Vector.fill 100000 1.2


create_random_text_vec =
random_vec = create_random_vec
random_vec = create_random_float_vec
random_vec.map .to_text


create_uniform_text_vec =
uniform_vec = create_uniform_vec
uniform_vec = create_uniform_integer_vec
uniform_vec.map .to_text


Expand All @@ -36,9 +42,13 @@ collect_benches = Bench.build builder->

builder.group "Vector_Distinct" options group_builder->
group_builder.specify "Random_Integer_Vector_Distinct_v2" <|
data.random_vec.distinct
data.random_integer_vec.distinct
group_builder.specify "Uniform_Integer_Vector_Distinct" <|
data.uniform_vec.distinct
data.uniform_integer_vec.distinct
group_builder.specify "Random_Float_Vector_Distinct_v2" <|
data.random_float_vec.distinct
group_builder.specify "Uniform_Float_Vector_Distinct" <|
data.uniform_float_vec.distinct
group_builder.specify "Random_Text_Vector_Distinct_v2" <|
data.random_text_vec.distinct
group_builder.specify "Uniform_Text_Vector_Distinct" <|
Expand Down
4 changes: 2 additions & 2 deletions test/Benchmarks/src/Vector/Operations.enso
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ options = Bench.options . set_warmup (Bench.phase_conf 5 5) . set_measure (Bench

collect_benches = Bench.build builder->
vector_size = 1000000
random_vec = Utils.make_random_vec vector_size
random_vec_2 = Utils.make_random_vec 100000
random_vec = Utils.make_random_float_vec vector_size
random_vec_2 = Utils.make_random_float_vec 100000
random_gen = Java_Random.new 123456
stateful_fun x =
s = State.get Number
Expand Down
8 changes: 4 additions & 4 deletions test/Benchmarks/src/Vector/Sort.enso
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@ type Data
create vec_size =
f1 s = Data.make_sorted_vec s
f2 s = make_partially_sorted_vec s
f3 s = Data.make_random_vec s
f4 s = (Data.make_random_vec s).map (v -> Int.Value v)
f3 s = Data.make_random_float_vec s
f4 s = (Data.make_random_float_vec s).map (v -> Int.Value v)
f5 s = Utils.make_random_rational_vec s
f6 s = f5 s . map (x-> x.to_float . floor + 2)
Data.Value (f1 vec_size) (f2 vec_size) (f3 vec_size) (f4 vec_size) (f5 vec_size) (f6 vec_size)

make_sorted_vec vec_size =
make_sorted_ascending_vec vec_size

make_random_vec vec_size =
Utils.make_random_vec vec_size
make_random_float_vec vec_size =
Utils.make_random_float_vec vec_size


collect_benches = Bench.build builder->
Expand Down
9 changes: 7 additions & 2 deletions test/Benchmarks/src/Vector/Utils.enso
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@ from Standard.Base import all

polyglot java import java.util.Random as Java_Random

make_random_vec : Integer -> Vector
make_random_vec n =
make_random_float_vec : Integer -> Vector
make_random_float_vec n =
random_gen = Java_Random.new n
Vector.new n (_-> random_gen.nextDouble)

make_random_integer_vec : Integer -> Vector
make_random_integer_vec n =
random_gen = Java_Random.new n
Vector.new n (_-> random_gen.nextInt)

make_random_rational_vec n =
random_gen = Java_Random.new n
Vector.new n _->
Expand Down
Loading