Add -WithIndex classes #72

mbid · 2017-06-15T13:03:50Z

Add -WithIndex classes

This adds FunctorWithIndex, FoldableWithIndex and
TraversableWithIndex and some combinators. Implementations are mostly
copied from the versions without index with minor adaptions. Similarly
for the documentation.

I think the corresponding haskell package does not state all laws for
these type classes, so there are TODO's in the doc strings of the three
classes.

StateL and StateR in TraversableWithIndex.purs are exact copies of
the versions in Traversable.purs because they are not exported there.
Maybe it would be better to merge TraversableWithIndex.purs into
Traversable.purs and similarly for Foldable. The Foldable and Traverse
instances of Array could then be along the lines of

traverse f = itraverse (const f)`

so that the foreign js imports for the unindexed version could be
removed.

FunctorWithIndex's imap collides with the imap of
purescript-invariant. Haskell calls the latter invmap. I don't know
whether that's a problem.

This does not yet include the -1WithIndex classes, i.e.
generalizations of Foldable1 and Traversable1 in Semigroup/. I
guess it would make sense to provide them here as well.

If this gets merged we should also implement instances in some other packages. purescript-lists and purescript-maps comes to mind. Anywhere else?

This adds `FunctorWithIndex`, `FoldableWithIndex` and `TraversableWithIndex` and some combinators. Implementations are mostly copied from the versions without index with minor adaptions. Similarly for the documentation. I think the corresponding haskell package does not state all laws for these type classes, so there are TODO's in the doc strings of the three classes. `StateL` and `StateR` in TraversableWithIndex.purs are exact copies of the versions in Traversable.purs because they are not exported there. Maybe it would be better to merge TraversableWithIndex.purs into Traversable.purs and similarly for Foldable. The Foldable and Traverse instances of Array could then be along the lines of ```purescript traverse f = itraverse (const f)` ``` so that the foreign js imports for the unindexed version could be removed. `FunctorWithIndex`'s `imap` collides with the `imap` of purescript-invariant. Haskell calls the latter `invmap`. I don't know whether that's a problem. This does not yet include the `-1WithIndex` classes, i.e. generalizations of `Foldable1` and `Traversable1` in `Semigroup/`. I guess it would make sense to provide them here as well.

garyb · 2017-06-18T13:05:20Z

src/Data/FunctorWithIndex.purs

+-- | - `imap (const f) = map f`
+-- | though?
+-- |
+-- | TODO: `imap` collides with `Invariant`s `imap`.


Maybe we should either use WithIndex suffixes on all of these functions, or prefix with ix as an alternative to i prefixes?

I wouldn't be too worried about overlapping with Invariant's map either though, there's always qualification. And Invariant is one of the less frequently used classes.

I'd prefer ix- over -WithIndex. Consistency with the haskell functions (so i-) is also nice I guess. Your call.

I'd prefer mapWithIndex since we use that convention elsewhere.

paf31 · 2017-06-18T15:47:05Z

Thanks!

I think we can remove the FFI requirement here by defining instances in terms of regular Foldable and Traversable and Array.mapWithIndex, right? (at the expense of a little performance admittedly)

What do you think?

paf31 · 2017-06-18T15:47:49Z

For the StateX thing, I think we could make an internal module.

mbid · 2017-06-19T07:01:30Z

Yeah, I agree that the duplication of the FFI implementations is not nice. I guess we can do two things:

What you said: Use imap, i.e. itraverse f = sequence <<< imap f. The FoldableWithIndex functions can be implemented either using Const and this itraverse, or imap Tuple followed by the folds without index. Unfortunately, we'd need to implement Const or Tuple here, because both packages already depend on foldable-traversable.
Implement the variants without index in terms of the indexed variants, e.g. foldMap = ifoldMap <<< const. This would require defining the -WithIndex classes in the same file together with the variants without index (e.g. FoldableWithIndex in Foldable.purs), or moving the FFI implementations to a separate internal module. Maybe foldMap would be a bit slower because of the additional argument passed, but ifoldMap would only iterate the array once instead of twice, and use no additional memory (similarly for the other functions).

I'd prefer the second version slightly, but let me know which one you'd find better.

mbid · 2017-06-21T19:02:04Z

I've just done some benchmarks for the second approach. Looks like this would make foldMap around 25% slower than it currently is.

EDIT: Added benchmarks for the other implementations as well.

the Traverse instances are slow anyway, and implementing itraverse in terms of traverse instead of natively (or vice-versa) does not change much, relatively.
Implementing foldr with ifoldr does not change worsen performance by much. On the other hand, ifoldr via foldr is only half as fast (I guess because the array is iterated twice instead of only once).
implementing foldMap in terms of ifoldMap makes it ~25% slower, ifoldMap via foldMap makes it ~18% slower.

So I guess we can definitely drop the foreign implementation of itraverse. I don't know what to do for foldMap. I don't know what to do with regard to foldMap. It's not big foreign implementation. We definitely need to keep the foreign implementation of imap (no other way to implement here).

Also fixes the analogous bug for ifoldMapDefaultL

The modified `Foldable` and `Traversable` laws that must hold for their -WithIndex versions are missing in the docs. But because the actual unmodified laws for `Foldable` and `Traversable` themselves are not here either I guess it shouldn't be a problem.

mbid · 2017-06-23T13:58:16Z

Ok, so I've removed the foreign implementations where possible. This means that ifoldr and ifoldl are only 1/2 - 1/3 as fast as they could to be for trivial iteration (e.g. just adding integers), but that's good for me. Given that the versions without indices are used far more often, I don't think it would've been a good tradeoff to take a performance hit for foldr instead of ifoldr.

Regarding the documentation, I've documented some laws that enforce that map f = imap (const f) and analogous equations for Foldable and Traversable.
Given that the complete Foldable and Traversable laws are not in the docs either, I found it pointless to document the modified versions for the -WithIndex versions. We should maybe improve this in a separate issue.

I guess I'll add some tests now. Can we break up the single test file into one for each class? As far as I can tell, there is no interdependence.

No tests for - ifoldM - itraverse_ - ifor_ - iall - iany - all instances except for Array These are not tested for Foldable either.

mbid · 2017-06-23T17:27:03Z

Ok so I've just gone ahead and added some tests. As far as I can tell, this should be OK to merge now unless we want to change the prefix of the indexed functions from i- to something else.

paf31 · 2017-06-27T03:21:03Z

src/Data/FunctorWithIndex.purs

+-- | map f = imap (const f)
+-- | ```
+class Functor f <= FunctorWithIndex i f | f -> i where
+  imap :: forall a b. (i -> a -> b) -> f a -> f b


This is called mapWithIndex elsewhere actually.

paf31 · 2017-06-27T03:23:02Z

@garyb Any more thoughts on this? Other than naming, I think this looks great.

mbid · 2017-06-27T22:26:24Z

Are you suggesting renaming only imap to mapWithIndex, or also ifoldMap to foldMapWithIndex and so on? I'd find it a little inconsistent if we only did the first thing.

paf31 · 2017-06-27T23:11:31Z

I'm suggesting renaming them all.

mbid · 2017-07-09T12:44:32Z

Ok, done.

paf31 · 2017-07-09T20:09:27Z

src/Data/Bifoldable.purs

-  bifoldr :: forall a b c. (a -> c -> c) -> (b -> c -> c) -> c -> p a b -> c
-  bifoldl :: forall a b c. (c -> a -> c) -> (c -> b -> c) -> c -> p a b -> c
-  bifoldMap :: forall m a b. Monoid m => (a -> m) -> (b -> m) -> p a b -> m
+  bfoldrWithIndex :: forall a b c. (a -> c -> c) -> (b -> c -> c) -> c -> p a b -> c


I'm confused why these changed.

Uh yeah, and rightly so.

paf31 · 2017-07-10T00:25:36Z

Great work, thank you!

I'll make a minor release for this since nothing seems to be breaking here.

Martin Bidlingmaier added 3 commits June 15, 2017 14:59

Fix export lists

3aa74e7

Add missing semicolon in js

a9612ad

garyb reviewed Jun 18, 2017

View reviewed changes

Martin Bidlingmaier added 5 commits June 23, 2017 11:45

Remove foreign implementation of array's itraverse

5bd1583

Move Accum and State to separate module

868bf0a

Remove foreign implementation of array's folds

9480573

Merge remote-tracking branch 'upstream/master' into with-index

a47d04c

Also fixes the analogous bug for ifoldMapDefaultL

Settle with incomplete laws

87ab449

The modified `Foldable` and `Traversable` laws that must hold for their -WithIndex versions are missing in the docs. But because the actual unmodified laws for `Foldable` and `Traversable` themselves are not here either I guess it shouldn't be a problem.

Martin Bidlingmaier added 2 commits June 23, 2017 18:18

Add some test for FoldableWithIndex

499e612

No tests for - ifoldM - itraverse_ - ifor_ - iall - iany - all instances except for Array These are not tested for Foldable either.

Add test for Array's TraversableWithIndex instance

c32ae6b

paf31 reviewed Jun 27, 2017

View reviewed changes

Martin Bidlingmaier added 2 commits July 9, 2017 14:26

Fix name: Add mising i- prefix

1a6ff7e

Change index naming scheme i- -> -WithIndex

b7ab13f

paf31 reviewed Jul 9, 2017

View reviewed changes

Revert accidentally renamed Bifoldable functions

5ee4cdf

paf31 merged commit c4875cc into purescript:master Jul 10, 2017

Add -WithIndex classes #72

Add -WithIndex classes #72

Uh oh!

Conversation

mbid commented Jun 15, 2017

Uh oh!

garyb Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

mbid Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

paf31 Jun 27, 2017

Choose a reason for hiding this comment

Uh oh!

paf31 commented Jun 18, 2017

Uh oh!

paf31 commented Jun 18, 2017

Uh oh!

mbid commented Jun 19, 2017

Uh oh!

mbid commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbid commented Jun 23, 2017

Uh oh!

mbid commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paf31 Jun 27, 2017

Choose a reason for hiding this comment

Uh oh!

paf31 commented Jun 27, 2017

Uh oh!

mbid commented Jun 27, 2017

Uh oh!

paf31 commented Jun 27, 2017

Uh oh!

mbid commented Jul 9, 2017

Uh oh!

paf31 Jul 9, 2017

Choose a reason for hiding this comment

Uh oh!

mbid Jul 9, 2017

Choose a reason for hiding this comment

Uh oh!

paf31 commented Jul 10, 2017

Uh oh!

Uh oh!

mbid commented Jun 21, 2017 •

edited

Loading

mbid commented Jun 23, 2017 •

edited

Loading