#532 AvgOf() bug on Double edge values fixed #543

allanwsilva · 2018-01-10T03:42:31Z

Fixing Issue #532

After reading @vmotsak comments and also considering @svendiedrichsen input and reading a few more things on the theme, like here, here and here, I decided to go on the simplest solution for now, implementing what @vmotsak suggested.

Maybe in the future we can consider adding more number types of avg. calculation (as @svendiedrichsen suggested).

0crat · 2018-01-10T03:42:40Z

@yegor256 please, pay attention to this pull request

0crat · 2018-01-10T03:42:54Z

Job #543 is now in scope, role is REV

fabriciofx · 2018-01-10T03:50:46Z

@AllanWS Before submit your PR, please run an $ mvn clean install -Pqulice to check if everything is fine. And another tip: more explicit issue title and commit messages (What kind of bugfix? Bugfix who?) helps a lot! 😄

codecov-io · 2018-01-10T04:40:49Z

Codecov Report

Merging #543 into master will increase coverage by 0.77%.
The diff coverage is 91.78%.

@@             Coverage Diff              @@
##             master     #543      +/-   ##
============================================
+ Coverage     66.27%   67.04%   +0.77%     
- Complexity      931      933       +2     
============================================
  Files           194      194              
  Lines          3208     3104     -104     
  Branches        245      207      -38     
============================================
- Hits           2126     2081      -45     
+ Misses         1013      970      -43     
+ Partials         69       53      -16

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/scalar/AvgOf.java	`92.5% <91.78%> (+27.82%)`	`43 <42> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c34b676...b74508d. Read the comment docs.

fabriciofx · 2018-01-10T04:47:56Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

@@ -178,4 +178,36 @@ public void withScalars() {
        );
    }

+    @Test
+    public void withDoubleMaxValue() {


@AllanWS I'm sorry, but your PR don't fix the same problem with Integer, Float and Long.

@fabriciofx , it's now fixed. Thanks for your "quick" review.

@AllanWS You're welcome.

…or all Java Number types (Float.MAX_VALUE, Integer.MAX_VALUE, Long.MAX_VALUE)

fabriciofx · 2018-01-10T07:01:02Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

+    public void withMaxValueCollections() {
+        MatcherAssert.assertThat(
+            new AvgOf(
+                new ListOf<>(Double.MAX_VALUE, Double.MAX_VALUE)


@AllanWS It would be nice if you test to just one, two and three MAX_VALUE (same to Double, Float, Long and Integer). Also change with odd and even values (max values you can). And to negative values? Any tests?

svendiedrichsen · 2018-01-10T08:03:07Z

src/main/java/org/cactoos/scalar/AvgOf.java

+            double sum = 0.0d;
+            double total = 1.0d;
+            for (final double val : src) {
+                sum += (val - sum) / total;
                total += 1.0f;
            }
            if (total == 0.0f) {
                total = 1.0f;
            }


Aren't these IFs obsolete?

@svendiedrichsen yes, you're right! Thanks.

svendiedrichsen · 2018-01-10T18:14:57Z

@AllanWS Could you test the average of new Double[]{100.0, 100.266} please? Should be 100.133 of course. But it isn't.

0crat · 2018-01-11T13:12:52Z

This pull request #543 is assigned to @fabriciofx. The budget is 15 minutes, see §4. Please, read §27 and §10 and when you decide to accept the changes, inform @yegor256 right in this ticket.

fabriciofx · 2018-01-12T01:12:42Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            int total = 0;
-            for (final int val : src) {
-                sum += val;
+            double sum = 0;


@AllanWS You should be explicit with double values. So, use 0.0d instead of 0.

fabriciofx · 2018-01-12T01:13:19Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            for (final int val : src) {
-                sum += val;
+            double sum = 0;
+            double total = 1;


@AllanWS The same here (1.0d).

fabriciofx · 2018-01-12T01:14:59Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            long total = 0L;
-            for (final long val : src) {
-                sum += val;
+            double sum = 0d;


@AllanWS The same here. See above.

fabriciofx · 2018-01-12T01:18:07Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

@@ -178,4 +178,86 @@ public void withScalars() {
        );
    }

+    @Test
+    public void withMaxValueCollections() {


@AllanWS I think interesting include more tests, like 100.0 and 100.266 as suggested by @svendiedrichsen, and Double.MAX_VALUE and 0, the last three and four Double.MAX_VALUE numbers (Double.MAX_VALUE, Double.MAX_VALUE - 1 and Double.MAX_VALUE - 2 for example.

fabriciofx · 2018-01-12T01:18:35Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

+        );
+        MatcherAssert.assertThat(
+            new AvgOf(
+                new ListOf<>(Integer.MAX_VALUE, Integer.MAX_VALUE)


@AllanWS The same here. See above.

fabriciofx · 2018-01-12T01:18:47Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

+        );
+        MatcherAssert.assertThat(
+            new AvgOf(
+                new ListOf<>(Float.MAX_VALUE, Float.MAX_VALUE)


@AllanWS The same here. See above.

fabriciofx · 2018-01-12T01:19:29Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

+    public void withMinValueCollections() {
+        MatcherAssert.assertThat(
+            new AvgOf(
+                new ListOf<>(Double.MIN_VALUE, Double.MIN_VALUE)


@AllanWS And Double.MAX_VALUE and Double.MIN_VALUE? See above.

fabriciofx · 2018-01-12T01:20:36Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

+
+    @Test
+    public void withEvenOddValues() {
+        final Float[] array = new ListOf<>(12345.0f, -12345.0f)


@AllanWS Prefer more clear names. Instead of array use numbers.

fabriciofx · 2018-01-12T01:22:16Z

src/main/java/org/cactoos/scalar/AvgOf.java

@@ -83,16 +83,13 @@ public AvgOf(final Integer... src) {
            }


@AllanWS First of all, I think it a good idea look at the @svendiedrichsen suggestion and comments about this issue.

fabriciofx · 2018-01-12T01:23:25Z

src/main/java/org/cactoos/scalar/AvgOf.java

@@ -83,16 +83,13 @@ public AvgOf(final Integer... src) {
            }
            return sum / total;
        }, () -> {
-            int sum = 0;


@AllanWS I don't know if is a good idea change int, long and float calculations for double. Is not possible use int, long and float? If not, why not?

fabriciofx · 2018-01-14T23:49:58Z

@AllanWS Please, check my comments above. I'll accept your PR when these problems will fixed, ok?

allanwsilva · 2018-01-15T00:15:57Z

@fabriciofx I'm still working on it.

… 532

…chsen

…chsen code polish (nice and clean) and added more test cases according @fabriciofx comments. Decimal precision problem solved as well.

allanwsilva · 2018-01-19T01:40:00Z

@fabriciofx please resume the code review.

fabriciofx · 2018-01-19T02:41:27Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            return sum / total;
-        });
+        super(() -> avgArray(
+            number -> BigDecimal.valueOf(number.intValue()), src


@AllanWS Needs indent.

fabriciofx · 2018-01-19T02:42:16Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            return sum / total;
-        });
+        super(() -> avgArray(
+            number -> BigDecimal.valueOf(number.longValue()), src


@AllanWS Needs indent.

fabriciofx · 2018-01-19T02:42:26Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            return sum;
-        });
+        super(() -> avgArray(
+            number -> BigDecimal.valueOf(number), src


@AllanWS Needs indent.

fabriciofx · 2018-01-19T02:43:53Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            return sum / total;
-        });
+        super(() -> avgArray(
+            number -> BigDecimal.valueOf(number.floatValue()), src


@AllanWS Needs indent.

fabriciofx · 2018-01-19T02:44:02Z

src/main/java/org/cactoos/scalar/AvgOf.java

-            return sum / total;
-        });
+        super(() -> avgIterator(
+            number -> BigDecimal.valueOf(number.longValue()), src


@AllanWS Needs indent.

fabriciofx · 2018-01-19T03:13:12Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

-                new ListOf<>(1.0f, 2.0f, 3.0f, 4.0f).toArray(new Float[4])
-            ).doubleValue(),
-            Matchers.equalTo(2.5d)
+                100.0, 100.666, 100.0


@AllanWS These values are double and you're checking if the result is float. Is is right?

fabriciofx · 2018-01-19T03:13:21Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

            ).floatValue(),
-            Matchers.equalTo(2.5f)
+            Matchers.equalTo(100.2665f)


@AllanWS These values are double and you're checking if the result is float. Is is right?

you can down/up cast numbers, this is "usual"...

fabriciofx · 2018-01-19T03:18:04Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

-        MatcherAssert.assertThat(
-            new AvgOf(array).intValue(), Matchers.equalTo(expected)
-        );
+    public void withIterableOfScalars() {


@AllanWS Please, add more tests, including using composition with other Scalar classes, for example:

new AvgOf( new MaxOf(2, 3), new MinOf(5, 9), new SumOf(1, 2, 3) ).intValue()

And don't forget the Ternary class too.

fabriciofx · 2018-01-19T03:21:44Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

@@ -51,213 +51,313 @@ public void withEmptyCollection() {
    }

    @Test
-    public void withIntegerCollection() {
+    public void withIntCollectionIntValue() {


@AllanWS Have you looked at other frameworks that work which values they used to test how calculate an average? Almost all tests are using only 1, 2, 3 and 4. It's important check with other numbers.

fabriciofx · 2018-01-19T03:22:59Z

src/test/java/org/cactoos/scalar/AvgOfTest.java

-                new ListOf<>(1.0d, 2.0d, 3.0d, 4.0d).toArray(new Double[4])
-            ).floatValue(),
-            Matchers.equalTo(2.5f)
+                Double.MAX_VALUE, Double.MAX_VALUE


@AllanWS Add tests using positive and negative numbers. And positive or negative and with NaN numbers.

They are already there, have a look again.

allanwsilva · 2018-01-19T05:19:51Z

@yegor256 is it mandatory the class org.cactoos.scalar.AvgOf to extend org.cactoos.scalar.NumberEnvelope?

I don't see a good design in those classes extending org.cactoos.scalar.NumberEnvelope, a lot of code duplication in the constructors (MinOf, SumOf, others) just to satisfy the super class constructor...

Is this open for discussion? Maybe this might help this discussion.

allanwsilva · 2018-01-19T05:40:19Z

@0crat waiting (here)

0crat · 2018-01-20T11:55:42Z

@0crat waiting (here) (here)

@AllanWS It's a code review job #543, can't put it on hold

yegor256 · 2018-01-22T19:40:37Z

@0crat status

0crat · 2018-01-22T19:40:58Z

@0crat status (here)

@yegor256 This is what I know about this job, as in §32:

The job #543 is in scope
The job is assigned to @fabriciofx /z
These users are banned: @AllanWS /z /z: This user reported the ticket

fabriciofx · 2018-01-25T00:42:28Z

@0crat status

0crat · 2018-01-25T00:42:46Z

@0crat status (here)

@fabriciofx This is what I know about this job, as in §32:

The job #543 is in scope for 15days
The role is REV
The job is assigned to @fabriciofx /z for 13days
These users are banned: @AllanWS /z: This user reported the ticket

0crat · 2018-01-29T13:16:52Z

Order was successfully finished: +15 points just awarded to @fabriciofx /z, total is +405

0crat · 2018-01-29T13:17:31Z

Payment to ARC for a closed pull request, as in §28: +15 points just awarded to @yegor256 /z, total is +8755

0crat · 2018-01-29T13:24:32Z

The job #543 is now out of scope

#532 bugfix

753bfeb

allanwsilva changed the title ~~#532 bugfix~~ #532 AvgOf() Double.MAX_VALUE Double.MIN_VALUE bug fixed Jan 10, 2018

Fixing code formatting issues..

cb8bfa4

allanwsilva changed the title ~~#532 AvgOf() Double.MAX_VALUE Double.MIN_VALUE bug fixed~~ #532 AvgOf() bug on Double edge values fixed Jan 10, 2018

fabriciofx reviewed Jan 10, 2018

View reviewed changes

Same mean calculation applied on all Avg ctors to handle max values f…

91c4467

…or all Java Number types (Float.MAX_VALUE, Integer.MAX_VALUE, Long.MAX_VALUE)

fabriciofx reviewed Jan 10, 2018

View reviewed changes

svendiedrichsen reviewed Jan 10, 2018

View reviewed changes

allanwsilva added 2 commits January 10, 2018 19:12

Just added a few more unit tests as @fabriciofx suggested

48bbdda

Fix as per @svendiedrichsen comment

a607fc9

refactored AvgOf to prevent precision and overflow errors.

358b7f1

fabriciofx reviewed Jan 12, 2018

View reviewed changes

allanwsilva added 3 commits January 18, 2018 17:12

Merge branch 'master' into 532

ea277e7

Merge branch 'avg' of https://github.com/svendiedrichsen/cactoos into…

6913187

… 532

AvgOf reviewed and finished for now. Refactored following @svendiedri…

b74508d

…chsen code polish (nice and clean) and added more test cases according @fabriciofx comments. Decimal precision problem solved as well.

fabriciofx reviewed Jan 19, 2018

View reviewed changes

allanwsilva mentioned this pull request Jan 20, 2018

AvgOf produces overflow for large values #532

Closed

rexim mentioned this pull request Jan 26, 2018

#532 Declarative fix of AvgOf() bug on Double edge values #581

Merged

rultor merged commit b74508d into yegor256:master Jan 29, 2018

#532 AvgOf() bug on Double edge values fixed #543

#532 AvgOf() bug on Double edge values fixed #543

Conversation

allanwsilva commented Jan 10, 2018 • edited Loading

0crat commented Jan 10, 2018

0crat commented Jan 10, 2018

fabriciofx commented Jan 10, 2018

codecov-io commented Jan 10, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svendiedrichsen commented Jan 10, 2018

0crat commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriciofx commented Jan 14, 2018

allanwsilva commented Jan 15, 2018

allanwsilva commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanwsilva commented Jan 19, 2018 • edited Loading

allanwsilva commented Jan 19, 2018

0crat commented Jan 20, 2018

yegor256 commented Jan 22, 2018

0crat commented Jan 22, 2018

fabriciofx commented Jan 25, 2018

0crat commented Jan 25, 2018

0crat commented Jan 29, 2018

0crat commented Jan 29, 2018

0crat commented Jan 29, 2018

allanwsilva commented Jan 10, 2018 •

edited

Loading

codecov-io commented Jan 10, 2018 •

edited

Loading

allanwsilva commented Jan 19, 2018 •

edited

Loading