Skip to content

[BEAM-413] - Improves test for floating point equality.#591

Closed
lucasamorimca wants to merge 1 commit intoapache:masterfrom
lucasamorimca:beam-413
Closed

[BEAM-413] - Improves test for floating point equality.#591
lucasamorimca wants to merge 1 commit intoapache:masterfrom
lucasamorimca:beam-413

Conversation

@lucasamorimca
Copy link
Contributor

This operation compares two floating point values for equality. Because
floating point calculations may involve rounding, calculated float and
double values may not be accurate. For values that must be precise, such
as monetary values, consider using a fixed-precision type such as
BigDecimal. For values that need not be precise, consider comparing for
equality within some range, for example: if ( Math.abs(x - y) < .0000001
).

For more information: https://issues.apache.org/jira/browse/BEAM-413

This operation compares two floating point values for equality. Because
floating point calculations may involve rounding, calculated float and
double values may not be accurate. For values that must be precise, such
as monetary values, consider using a fixed-precision type such as
BigDecimal. For values that need not be precise, consider comparing for
equality within some range, for example: if ( Math.abs(x - y) < .0000001
).

For more information: https://issues.apache.org/jira/browse/BEAM-413
@robertwb
Copy link
Contributor

robertwb commented Jul 6, 2016

Thanks for your pull request.

The choice of 1e-7 here as a cutoff is arbitrary here; when choosing a bound for equality one needs to consider what makes sense in context, and there really isn't any context to go on here, thus I think the only correct thing to do here is leave exact equality. (Either that or simply remove the equals() method on CountSum, it's really not needed.)

}
@SuppressWarnings("unchecked")
CountSum<?> otherCountSum = (CountSum<?>) other;
return (count == otherCountSum.count)
Copy link
Contributor

Choose a reason for hiding this comment

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

count is an integer

@dhalperi
Copy link
Contributor

dhalperi commented Jul 6, 2016

Sensible options here:

  • Delete equals and hashCode -- these need not exist, I think.
  • Rewrite equals to check relative rather than absolute values. Aka, count1 == count2 && sum1 == 0.0 && sum2 == 0.0 or abs(sum1 - sum2)/min(abs(sum1), abs(sum2)) < 0.0000001
  • Do nothing.

All three of these are probably better solutions than the current PR.

@lucasamorimca
Copy link
Contributor Author

Thanks for the feedback :)

@kennknowles
Copy link
Member

+1 to removing equals. It should only every be used in tests, if anywhere, and there it should be "equals up to error bound" in context like @robertwb says.

@dhalperi
Copy link
Contributor

dhalperi commented Jul 7, 2016

@lucasallan -- thanks for picking this bug and all the other starter issues you fixed recently; excited to have your contributions! Hope you're willing to try again in this PR if interested.

@lucasamorimca
Copy link
Contributor Author

@dhalperi yes, I'll pick this up again and I also have other issues in my queue to work on. Looking forward for more of your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants