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

Monoid[Double] and Monoid[Float] are incorrect #301

Closed
xuwei-k opened this issue May 4, 2015 · 3 comments
Closed

Monoid[Double] and Monoid[Float] are incorrect #301

xuwei-k opened this issue May 4, 2015 · 3 comments
Assignees

Comments

@xuwei-k
Copy link
Contributor

xuwei-k commented May 4, 2015

xuwei-k@6dbad2ddff10

@ceedubs
Copy link
Contributor

ceedubs commented Sep 28, 2015

I believe this was a conscious decision by @non with the assumption that people generally know about the gotchas with floating point arithmetic but still want to be able to add floats and doubles. Is that an accurate assessment, @non?

It seems to me like the options are either:

  1. Move these into alleycats (but I suspect people using algebra without Cats still want these)
  2. Be explicit about the fact that we know this is naughty but we are doing it anyway. In this case we should add some documentation about this.

I could see an argument that the blame lies as much with the Eq instances as it does with the Semigroup and Monoid instances.

@xuwei-k
Copy link
Contributor Author

xuwei-k commented Sep 28, 2015

I don't mean it.

https://github.com/non/cats/blob/682a39423bca461ffb37a0ff7d12b2a6fda7099e/core/src/main/scala/cats/std/anyval.scala#L97-L98

def combine(x: Float, y: Float): Float = x + y
def empty: Float = 1F

the combine defined by +, but empty is 1F.
I think empty should be 0F or def combine(x: Float, y: Float): Float = x * y

@ceedubs
Copy link
Contributor

ceedubs commented Sep 28, 2015

@xuwei-k oh wow thanks for pointing that out. I can't believe we let that go for so long! I'll submit a fix.

ceedubs pushed a commit to ceedubs/cats that referenced this issue Sep 28, 2015
Fixes typelevel#301.

This was ignored for far too long (even though there was typelevel#301 for it).
We should really have test coverage on this. I say we merge this for now
but discuss whether this should just be coming from algebra and add
tests to whichever project it comes from.
@ceedubs ceedubs self-assigned this Sep 28, 2015
@non non closed this as completed in #547 Sep 28, 2015
@non non removed the in progress label Sep 28, 2015
julienrf pushed a commit to julienrf/cats that referenced this issue Oct 9, 2015
Fixes typelevel#301.

This was ignored for far too long (even though there was typelevel#301 for it).
We should really have test coverage on this. I say we merge this for now
but discuss whether this should just be coming from algebra and add
tests to whichever project it comes from.
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

No branches or pull requests

3 participants