Skip to content

Change reexports to specialize to type Array #173

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

Closed
wants to merge 1 commit into from

Conversation

jvliwanag
Copy link

Wondering if the reexports should specialize to Array. That way, in my application code, Array.intercalate guarantees that I'm working with an Array.

This also helps with potential performance improvements by custom implementations later on that are specialized to Array.

@natefaubion
Copy link

natefaubion commented Jun 4, 2020

Even though this is a breaking change, this has irritated me since forever and I would like this to be merged. I have never wanted to import generic foldable methods from a monomorphic library. It just makes no sense. It also means all these methods are necessarily and always worst case due to Foldable, so we rely on irritating hacks like findIndex followed by index.

@garyb
Copy link
Member

garyb commented Jun 4, 2020

I remember when we added these as re-exports in the first place I wasn't super enthusiastic about it, as I felt like it might obscure the fact that these operations are generic, and yeah, lead to some weird import/usage seeming contradictions.

I could understand a little bit if they were implemented more efficiently as per Nate's point, but I'm not a fan of monomorphizing them just for the sake of re-exporting. I'd rather remove the re-exports than do this, personally.

@natefaubion
Copy link

I think that there are probably quite a few usages in the wild that are appropriately monomorphic. I agree with you, but continuing to export them breaks less code.

@hdgarrood
Copy link
Contributor

Are the Foldable versions of things really that bad in terms of performance here? Certainly the methods which are members of the Foldable class - that's foldl, foldr, foldMap - can be assumed to be equally as good as monomorphic functions because they essentially are. I wouldn't expect the difference between foldMap identity (which is how fold is defined) and a specialized version of fold to be that great. Note that any and all can't short-circuit because they are working with an arbitrary HeytingAlgebra so I wouldn't expect much difference with an Array-specialized version there. I feel like the only ones where I might expect more of a difference are the functions elem, notElem, find, and findMap, because they can't short-circuit once an element has been found, and scanl and scanr, because expressing them in terms of Traversable is a bit awkward.

@JordanMartinez
Copy link
Contributor

To clarify, is the next action on this PR just fixing the conflicting files?

@jvliwanag jvliwanag force-pushed the specialized-reexport branch from a9d4e97 to 3e2ddd2 Compare November 27, 2020 06:05
@hdgarrood
Copy link
Contributor

I would prefer to replace the implementations of the functions I listed with versions which are implemented in terms of arrays specifically for performance - that is, elem, notElem, find, findMap, scanl, and scanr.

@natefaubion
Copy link

natefaubion commented Nov 27, 2020

Note that any and all can't short-circuit because they are working with an arbitrary HeytingAlgebra so I wouldn't expect much difference with an Array-specialized version there.

I think that the monomorphic versions should be specialized to Boolean. You are correct, there is no performance advantage since they cannot short circuit (at least not without additional, dubious constraints), so having these monomorphic to Array but still generic over HeytingAlgebra gives us nothing. However, specializing it to Boolean is by far the most common case (I've never personally taken advantage of HeytingAlgebra generality) and I think beneficial. If you want that generality you can continue to use the Foldable implementations.

@kl0tl
Copy link
Member

kl0tl commented Dec 17, 2020

Do we still need this since #189 has been merged? For specialized short-circuiting any, all :: (a -> Boolean) -> Array a -> Boolean functions perhaps?

@JordanMartinez
Copy link
Contributor

I'd say let's specialize all and any to Array and Boolean. As Nate pointed out, someone can use Foldable if they need the HeytingAlgebra constraint.

@kl0tl
Copy link
Member

kl0tl commented Dec 19, 2020

I opened #193.

@hdgarrood
Copy link
Contributor

I think let’s close this for now then, since we’re going the route of providing optimised specialised versions rather than just specialising but keeping the same implementation. Thanks anyway for getting the ball rolling on this @jvliwanag!

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.

6 participants