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.

Sign up for GitHub

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

Jump to bottom

Clarify rounding semantics #49

Closed
gunnarmorling opened this issue Jan 3, 2024 · 3 comments
Closed

Clarify rounding semantics #49

gunnarmorling opened this issue Jan 3, 2024 · 3 comments

Comments

@gunnarmorling
Copy link
Owner

See #5 (comment); CC @AlexanderYastrebov
@gunnarmorling gunnarmorling mentioned this issue Jan 3, 2024
Merged
@ddimtirov
Copy link
Contributor

I find that Math.round((double) x / y) / 10.0 produces the same result as the reference - the important part is to do the division in double - addition is guaranteed to be loseless.

Also, I have redone the reference impl as BigDecimal with rounding half-even, and the results don't change (for my file).

CalculateAverage_BigDecimal.java.txt

AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 4, 2024
@AlexanderYastrebov
wip,test: add rounding test case
1cc9910 
Added a test case from the discussion gunnarmorling#5 (comment)

Neither all implementations match baseline:
```sh
$ ./test_all.sh src/test/resources/samples/measurements-rounding-baseline.txt 2>/dev/null
FAIL armandino
FAIL artsiomkorzun
PASS baseline
PASS bjhara
PASS criccomini
FAIL ddimtirov
FAIL ebarlas
FAIL filiphr
FAIL itaske
PASS jgrateron
FAIL khmarbaise
FAIL kuduwa-keshavram
PASS lawrey
PASS moysesb
FAIL nstng
PASS padreati
FAIL palmr
PASS richardstartin
FAIL royvanrijn
PASS seijikun
PASS spullara
PASS truelive
```

not they match precise value `33.6+31.7+21.9+14.6=25.5`:
```
$ ./test_all.sh src/test/resources/samples/measurements-rounding-precise.txt 2>/dev/null
PASS armandino
PASS artsiomkorzun
FAIL baseline
FAIL bjhara
FAIL criccomini
FAIL ddimtirov
PASS ebarlas
PASS filiphr
PASS itaske
FAIL jgrateron
PASS khmarbaise
FAIL kuduwa-keshavram
FAIL lawrey
FAIL moysesb
PASS nstng
FAIL padreati
PASS palmr
FAIL richardstartin
PASS royvanrijn
FAIL seijikun
FAIL spullara
FAIL truelive
```

For gunnarmorling#49
AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 4, 2024
@AlexanderYastrebov
wip,test: add rounding test case
7e9743a 
Added a test case from the discussion gunnarmorling#5 (comment)

Neither all implementations match baseline:
nor they match precise value of `33.6+31.7+21.9+14.6=25.5`:
```
$ ./test_all.sh src/test/resources/samples/measurements-rounding-baseline.txt 2>/dev/null | tee /tmp/rounding-baseline.log
FAIL armandino
FAIL artsiomkorzun
PASS baseline
PASS bjhara
PASS criccomini
FAIL ddimtirov
FAIL ebarlas
FAIL filiphr
FAIL itaske
PASS jgrateron
FAIL khmarbaise
FAIL kuduwa-keshavram
PASS lawrey
PASS moysesb
FAIL nstng
PASS padreati
FAIL palmr
PASS richardstartin
FAIL royvanrijn
PASS seijikun
PASS spullara
PASS truelive

$ ./test_all.sh src/test/resources/samples/measurements-rounding-precise.txt 2>/dev/null | tee /tm
p/rounding-precise.log
PASS armandino
PASS artsiomkorzun
FAIL baseline
FAIL bjhara
FAIL criccomini
FAIL ddimtirov
PASS ebarlas
PASS filiphr
FAIL itaske
FAIL jgrateron
PASS khmarbaise
FAIL kuduwa-keshavram
FAIL lawrey
FAIL moysesb
PASS nstng
FAIL padreati
PASS palmr
FAIL richardstartin
PASS royvanrijn
FAIL seijikun
FAIL spullara
FAIL truelive

$ git --no-pager diff --word-diff /tmp/rounding-baseline.log /tmp/rounding-precise.log
diff --git a/tmp/rounding-baseline.log b/tmp/rounding-precise.log
index 76d5b4e..495fb00 100644
--- a/tmp/rounding-baseline.log
+++ b/tmp/rounding-precise.log
@@ -1,22 +1,22 @@
[-FAIL-]{+PASS+} armandino[-FAIL artsiomkorzun-]
PASS {+artsiomkorzun+}
{+FAIL+} baseline
[-PASS-]{+FAIL+} bjhara
[-PASS-]{+FAIL+} criccomini
FAIL ddimtirov
[-FAIL-]{+PASS+} ebarlas
[-FAIL-]{+PASS+} filiphr
FAIL itaske
[-PASS jgrateron-]FAIL {+jgrateron+}
{+PASS+} khmarbaise
FAIL kuduwa-keshavram
[-PASS-]{+FAIL+} lawrey[-PASS moysesb-]
FAIL [-nstng-]{+moysesb+}
PASS [-padreati-]{+nstng+}
FAIL [-palmr-]{+padreati+}
PASS [-richardstartin-]{+palmr+}
FAIL [-royvanrijn-]{+richardstartin+}
PASS {+royvanrijn+}
{+FAIL+} seijikun
[-PASS-]{+FAIL+} spullara
[-PASS-]{+FAIL+} truelive
```

For gunnarmorling#49
AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 4, 2024
@AlexanderYastrebov
wip,test: add rounding test case
107ca23 
Added two test cases from the discussion gunnarmorling#5 (comment)

Neither all implementations match baseline nor they match precise value of `33.6+31.7+21.9+14.6=25.5`:
```
$ ./test_all.sh src/test/resources/samples/measurements-rounding-baseline.txt 2>/dev/null | tee /tmp/rounding-baseline.log
FAIL armandino
FAIL artsiomkorzun
PASS baseline
PASS bjhara
PASS criccomini
FAIL ddimtirov
FAIL ebarlas
FAIL filiphr
FAIL itaske
PASS jgrateron
FAIL khmarbaise
FAIL kuduwa-keshavram
PASS lawrey
PASS moysesb
FAIL nstng
PASS padreati
FAIL palmr
PASS richardstartin
FAIL royvanrijn
PASS seijikun
PASS spullara
PASS truelive

$ ./test_all.sh src/test/resources/samples/measurements-rounding-precise.txt 2>/dev/null | tee /tm
p/rounding-precise.log
PASS armandino
PASS artsiomkorzun
FAIL baseline
FAIL bjhara
FAIL criccomini
FAIL ddimtirov
PASS ebarlas
PASS filiphr
FAIL itaske
FAIL jgrateron
PASS khmarbaise
FAIL kuduwa-keshavram
FAIL lawrey
FAIL moysesb
PASS nstng
FAIL padreati
PASS palmr
FAIL richardstartin
PASS royvanrijn
FAIL seijikun
FAIL spullara
FAIL truelive

$ git --no-pager diff --word-diff /tmp/rounding-baseline.log /tmp/rounding-precise.log
diff --git a/tmp/rounding-baseline.log b/tmp/rounding-precise.log
index 76d5b4e..495fb00 100644
--- a/tmp/rounding-baseline.log
+++ b/tmp/rounding-precise.log
@@ -1,22 +1,22 @@
[-FAIL-]{+PASS+} armandino[-FAIL artsiomkorzun-]
PASS {+artsiomkorzun+}
{+FAIL+} baseline
[-PASS-]{+FAIL+} bjhara
[-PASS-]{+FAIL+} criccomini
FAIL ddimtirov
[-FAIL-]{+PASS+} ebarlas
[-FAIL-]{+PASS+} filiphr
FAIL itaske
[-PASS jgrateron-]FAIL {+jgrateron+}
{+PASS+} khmarbaise
FAIL kuduwa-keshavram
[-PASS-]{+FAIL+} lawrey[-PASS moysesb-]
FAIL [-nstng-]{+moysesb+}
PASS [-padreati-]{+nstng+}
FAIL [-palmr-]{+padreati+}
PASS [-richardstartin-]{+palmr+}
FAIL [-royvanrijn-]{+richardstartin+}
PASS {+royvanrijn+}
{+FAIL+} seijikun
[-PASS-]{+FAIL+} spullara
[-PASS-]{+FAIL+} truelive
```

For gunnarmorling#49
AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 4, 2024
@AlexanderYastrebov
wip,test: add rounding test case
27cdd53 
Added two test cases from the discussion gunnarmorling#5 (comment)

Neither all implementations match baseline nor they match precise value of `33.6+31.7+21.9+14.6=25.5`:
```
$ ./test_all.sh src/test/resources/samples/measurements-rounding-baseline.txt 2>/dev/null | tee /tmp/rounding-baseline.log
FAIL armandino
FAIL artsiomkorzun
PASS baseline
PASS bjhara
PASS criccomini
FAIL ddimtirov
FAIL ebarlas
FAIL filiphr
FAIL itaske
PASS jgrateron
FAIL khmarbaise
FAIL kuduwa-keshavram
PASS lawrey
PASS moysesb
FAIL nstng
PASS padreati
FAIL palmr
PASS richardstartin
FAIL royvanrijn
PASS seijikun
PASS spullara
PASS truelive

$ ./test_all.sh src/test/resources/samples/measurements-rounding-precise.txt 2>/dev/null | tee /tm
p/rounding-precise.log
PASS armandino
PASS artsiomkorzun
FAIL baseline
FAIL bjhara
FAIL criccomini
FAIL ddimtirov
PASS ebarlas
PASS filiphr
FAIL itaske
FAIL jgrateron
PASS khmarbaise
FAIL kuduwa-keshavram
FAIL lawrey
FAIL moysesb
PASS nstng
FAIL padreati
PASS palmr
FAIL richardstartin
PASS royvanrijn
FAIL seijikun
FAIL spullara
FAIL truelive

$ diff -y /tmp/rounding-baseline.log /tmp/rounding-precise.log
FAIL armandino                                                | PASS armandino
FAIL artsiomkorzun                                            | PASS artsiomkorzun
PASS baseline                                                 | FAIL baseline
PASS bjhara                                                   | FAIL bjhara
PASS criccomini                                               | FAIL criccomini
FAIL ddimtirov                                                  FAIL ddimtirov
FAIL ebarlas                                                  | PASS ebarlas
FAIL filiphr                                                  | PASS filiphr
FAIL itaske                                                     FAIL itaske
PASS jgrateron                                                | FAIL jgrateron
FAIL khmarbaise                                               | PASS khmarbaise
FAIL kuduwa-keshavram                                           FAIL kuduwa-keshavram
PASS lawrey                                                   | FAIL lawrey
PASS moysesb                                                  | FAIL moysesb
FAIL nstng                                                    | PASS nstng
PASS padreati                                                 | FAIL padreati
FAIL palmr                                                    | PASS palmr
PASS richardstartin                                           | FAIL richardstartin
FAIL royvanrijn                                               | PASS royvanrijn
PASS seijikun                                                 | FAIL seijikun
PASS spullara                                                 | FAIL spullara
PASS truelive                                                 | FAIL truelive
```

Its also interesting that e.g. `itaske` produces different results
between runs and thus may pass or fail sporadically.

For gunnarmorling#49
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jan 4, 2024
Closed
Kidlike added a commit to Kidlike/1brc that referenced this issue Jan 10, 2024
@Kidlike
fix rounding (see gunnarmorling/issues/49)
2b1ce73
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jan 12, 2024
Merged
3 tasks
gunnarmorling pushed a commit that referenced this issue Jan 13, 2024
@Kidlike
submission for kidlike (#294)
c016684 
* first version

* second version (0m59s)

* third version (0m46s)

* fourth version (0m39s)

* fifth version (0m18s)

* follow naming conventions from project structure

* fix rounding (see /issues/49)

* formatting changes from build

* name should case-match github username

* sixth version (14s)

* seventh version (11s)

* potential fix for other systems?

* no need for sdk install

* binary should go to ./target

* building native-image only when not existing yet
dmitry-midokura pushed a commit to dmitry-midokura/1brc that referenced this issue Jan 13, 2024
@Kidlike @dmitry-midokura
submission for kidlike (gunnarmorling#294)
ae82450 
* first version

* second version (0m59s)

* third version (0m46s)

* fourth version (0m39s)

* fifth version (0m18s)

* follow naming conventions from project structure

* fix rounding (see gunnarmorling/issues/49)

* formatting changes from build

* name should case-match github username

* sixth version (14s)

* seventh version (11s)

* potential fix for other systems?

* no need for sdk install

* binary should go to ./target

* building native-image only when not existing yet
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jan 14, 2024
Closed
AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 14, 2024
@AlexanderYastrebov
Compare results with tolerance
926dc61 
As an exercise I've re-implemented gunnarmorling#375 idea to compare result numbers
with a tolerance.

But I think this approach is wrong, the rules should define rounding
mode and baseline should be fixed to produce the correct result.

Submissions should be fixed accordingly or qualified as non-passing.

Could have been done earlier but better late than never.

Updates gunnarmorling#49
AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 14, 2024
@AlexanderYastrebov
Compare results with tolerance
821f45d 
As an exercise I've re-implemented gunnarmorling#375 idea to compare result numbers
with a tolerance:
```
$ ./test.sh baseline 0 src/test/resources/samples/measurements-rounding-precise.txt
Validating calculate_average_baseline.sh -- src/test/resources/samples/measurements-rounding-precise.txt
Rounding=14.6/25.5/33.6 != Rounding=14.6/25.4/33.6 (avg)
```

But I think this approach is wrong, the rules should define rounding
mode and baseline should be fixed to produce the correct result.

Submissions should be fixed accordingly or qualified as non-passing.

Could have been done earlier but better late than never.

Updates gunnarmorling#49
AlexanderYastrebov added a commit to AlexanderYastrebov/1brc that referenced this issue Jan 14, 2024
@AlexanderYastrebov
Compare results with tolerance
7e63144 
As an exercise I've re-implemented gunnarmorling#375 idea to compare result numbers
with a tolerance:
```
$ ./test.sh baseline 0 src/test/resources/samples/measurements-rounding-precise.txt
Validating calculate_average_baseline.sh -- src/test/resources/samples/measurements-rounding-precise.txt
Rounding=14.6/25.5/33.6 != Rounding=14.6/25.4/33.6 (avg)
```
The hassle of parsing highlights the importance of using
machine-readable format as suggested in gunnarmorling#14

**But** I think this approach is wrong, the rules should define rounding
mode and baseline should be fixed to produce the correct result.

Submissions should be fixed accordingly or qualified as non-passing.

Could have been done earlier but better late than never.

Updates gunnarmorling#49
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jan 14, 2024
Closed
gunnarmorling added a commit that referenced this issue Jan 14, 2024
@gunnarmorling
#49 Fixing rounding behavior of baseline implementation
6cb1c03
gunnarmorling added a commit that referenced this issue Jan 14, 2024
@gunnarmorling
#49 Fixing rounding behavior of baseline implementation
f0204d9
gunnarmorling added a commit that referenced this issue Jan 14, 2024
@gunnarmorling
#49 Fixing rounding behavior of baseline implementation
ff71b64
gunnarmorling added a commit that referenced this issue Jan 14, 2024
@gunnarmorling
#49 Fixing rounding behavior of baseline implementation
3f1fd2e
gunnarmorling added a commit that referenced this issue Jan 14, 2024
@gunnarmorling
#49 Fixing rounding behavior of baseline implementation
7816e32
@gunnarmorling
Copy link
Owner Author

Done via #392.

@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jan 14, 2024
Merged
@AlexanderYastrebov
Copy link
Contributor

Done via #392.

Unfortunately no, see https://github.com/gunnarmorling/1brc/pull/392/files#r1451734941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gunnarmorling @ddimtirov @AlexanderYastrebov