Skip to content
This repository was archived by the owner on Nov 18, 2023. It is now read-only.

Added Monoid and Applicative Instances#27

Merged
Daniel-Diaz merged 5 commits intoDaniel-Diaz:masterfrom
octopuscabbage:master
Jan 27, 2016
Merged

Added Monoid and Applicative Instances#27
Daniel-Diaz merged 5 commits intoDaniel-Diaz:masterfrom
octopuscabbage:master

Conversation

@octopuscabbage
Copy link
Copy Markdown
Contributor

No description provided.

@octopuscabbage
Copy link
Copy Markdown
Contributor Author

This addresses part of #17

Data/Matrix.hs Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why not using the monoid instance for Maybe?

fromMaybe mempty $ safeGet row column m <> safeGet row column m'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, fixed that

@Daniel-Diaz
Copy link
Copy Markdown
Owner

Could you make the changes to tests a separate PR? I would like to review that independently.

Thanks for contributing!

@octopuscabbage
Copy link
Copy Markdown
Contributor Author

Do you mean the specific instances or changing the format of them? Because the format is already in this PR #21

I should've added that this one was downstream to that one.

@Daniel-Diaz
Copy link
Copy Markdown
Owner

I know you opened that PR, but the changes are in this PR as well. Check the "Files Changed" tab.

@Daniel-Diaz
Copy link
Copy Markdown
Owner

I checked the Applicative laws myself. They all seem to hold. Beautiful.

@Daniel-Diaz Daniel-Diaz self-assigned this Jan 26, 2016
@octopuscabbage
Copy link
Copy Markdown
Contributor Author

I think I get what you want. You want me to take the tests out of this and make them their own PR which would be downstream from the other test PR?

Perhaps someone smarter than me can attempt to get the quickcheck tests working for applicative, as someone smarter than me told me what the applicative instance should be.

There was some talk of a monad instance which I guess would be similar to the applicative instance except flattened twice but that one is a lot harder to test to make sure the monad laws work. I guess it would work similarly to how the list monad works.

@octopuscabbage
Copy link
Copy Markdown
Contributor Author

Alright now this can be merged independently of #21 and #28

Daniel-Diaz pushed a commit that referenced this pull request Jan 27, 2016
Added Monoid and Applicative Instances
@Daniel-Diaz Daniel-Diaz merged commit 4918834 into Daniel-Diaz:master Jan 27, 2016
@Daniel-Diaz
Copy link
Copy Markdown
Owner

Thanks a lot!

@octopuscabbage
Copy link
Copy Markdown
Contributor Author

np, always happy to contrib

@Daniel-Diaz
Copy link
Copy Markdown
Owner

About the monad instance

To write a monad instance we will need a function with the same type as flatten:

join :: Matrix (Matrix a) -> Matrix a

With the submatrices being allowed to have any size. I don't see a way to implement that in any reasonable way.

@octopuscabbage
Copy link
Copy Markdown
Contributor Author

This is the error I ran into. I don't think a monad is possible.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants