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 Semiring instance #59

Merged
merged 2 commits into from
Nov 10, 2021
Merged

Add Semiring instance #59

merged 2 commits into from
Nov 10, 2021

Conversation

ozkutuk
Copy link
Contributor

@ozkutuk ozkutuk commented Nov 6, 2021

Description of the change

Semiring instance was removed by #20 with response to #18, due to it not satisfying all the semiring laws. This PR re-adds the instance with an implementation that satisfies the laws. The instance passed all the tests provided by Test.QuickCheck.Laws.Data.Semiring. The implementation is roughly the same as the instance from the semirings package on Hackage.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez
Copy link
Contributor

In #18,

Changing the definition of zero to be Nothing could fix this but might break other laws, I'm not sure.

I'm assuming the 'other laws' are in reference to the Ring, ModuloRing, DivisionRing, and Num type classes that existed at that time.

By adding this instance back, are either type classes (e.g. Ring) now possible, too?

Otherwise, LGTM.

@ozkutuk
Copy link
Contributor Author

ozkutuk commented Nov 6, 2021

I don't think having a Ring instance is possible. It requires an additive inverse operation negate such that a + negate a = zero. However, since we have zero = Nothing, there is no reasonable way to have such an inverse:

let a = Just 5
    a' = negate a  -- Just (-5)
 in a + a'  -- Just 0 ≠ Nothing

And since all other ring related classes require Ring as their constraint, they can't be added either. Once again, taking semirings package of Haskell as reference, there is also no Ring instance for Maybe there.

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks for clarifying.

@JordanMartinez JordanMartinez merged commit f6b5294 into purescript:master Nov 10, 2021
@ozkutuk ozkutuk deleted the add-semiring-instance branch November 11, 2021 18:10
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.

3 participants