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 a Distributive instance for Solo #68

Open
wants to merge 1 commit into
base: 0
Choose a base branch
from
Open

Add a Distributive instance for Solo #68

wants to merge 1 commit into from

Conversation

RyanGlScott
Copy link
Collaborator

Inspired by a similar patch on the main branch from #65.

Fixes #67.

Inspired by a similar patch on the `main` branch from #65.

Fixes #67.
@treeowl
Copy link
Contributor

treeowl commented Dec 27, 2022

I ... guess? It's really awkward though.

distribute :: Functor f => f (Solo a) -> Solo (f a)

The only way to implement this is

distribute = Solo . fmap getSolo

fmap getSolo is a troubling concept when considering how Solo is used. It's not that the instance is wrong (it's not), but it's just really hard to think of a legitimate use for it.

@phadej
Copy link
Collaborator

phadej commented Dec 27, 2022

@treeowl I'm confused, you wrote #65? Isn't that needed after all? One needs this instances to write current Representable Solo instance (in adjunctions).

@treeowl
Copy link
Contributor

treeowl commented Dec 27, 2022

@treeowl I'm confused, you wrote #65? Isn't that needed after all? One needs this instances to write current Representable Solo instance (in adjunctions).

I don't remember. I hope I too am allowed to be of two minds, change my mind, and learn over time.

@phadej
Copy link
Collaborator

phadej commented Dec 27, 2022

I don't remember. I hope I too am allowed to be of two minds, change my mind, and learn over time.

Sure. That's why a nitpick on many changes for the motivation or the reasoning is there, so it's possible to remember.

The motivation here is to allow writing instance Representable Solo. To be fair, you added that instance in #65, not Distributive Solo, as Distributive is gone from the main of distributive. I hope it's easier to think for legitimate use of Representable Solo.

@treeowl
Copy link
Contributor

treeowl commented Dec 27, 2022

Representable Solo seems equally problematic to me. Then again, one could argue the same problems occur for any sort of "vector-like" type, so why make a fuss about this one? Sigh.

@phadej
Copy link
Collaborator

phadej commented Dec 27, 2022

Then again, one could argue the same problems occur for any sort of "vector-like" type,

So Representable V2 from linear is problematic. How? Or is it fine because it has bangs? Please explain.

@treeowl
Copy link
Contributor

treeowl commented Dec 27, 2022

It's all contextual, isn't it? Any Functor with strict fields in its parameter's type is automatically problematic because it doesn't strictly obey the functor laws. However, a Representable instance for a functor with lazy fields should generally be used for lazy memoization and not just filling in values; it can leak for the latter purpose. Maybe there's some missing abstraction for "things that can be filled eagerly"? That could have a method like

tabulateNow :: (Rep f -> Solo a) -> f a

but I have no idea if reasonable laws could be formulated for such a thing.

@phadej
Copy link
Collaborator

phadej commented Dec 27, 2022

Are you saying that Representable shouldn't be used for "parallel computing" like in APLicative Programming with
Naperian Functors
, or if it does then the functors should have strict fields?

It feels I lack a lot of background knowledge, so I guess I should not comment more.

@RyanGlScott you may drop this instance, and apparently the one in main as well. Especially if you understand what David is talking about and agree.

@treeowl
Copy link
Contributor

treeowl commented Dec 27, 2022

@phadej , @RyanGlScott , I want to be very clear that I'm not saying the instance shouldn't be added. I'm just saying I have misgivings about it.

@RyanGlScott
Copy link
Collaborator Author

I don't have strong feelings about this at all—I was merely submitting this to achieve parity with the main branch. If you have misgivings about the instance, I'd be happy to remove it from the main branch.

@aaronvargo
Copy link
Collaborator

I think #69 (which I've created in the context of the main branch, but also applies to the released one) may be what's bothering @treeowl, though I'm not sure

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