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.

Sign up for GitHub

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

Jump to bottom

indexed foldable-traversable instances #134

Merged
merged 10 commits into from
Nov 30, 2017
Merged

indexed foldable-traversable instances #134

merged 10 commits into from
Nov 30, 2017

Conversation

matthewleon
Copy link
Contributor

No description provided.

@paf31
Copy link
Contributor

paf31 commented Nov 22, 2017

Thanks!

Maybe we should deprecate Data.List.mapWithIndex now. Also, would you like to add TraversableWithIndex here too?

@matthewleon
Copy link
Contributor Author

Not sure how deprecation works in these libs, but I've done the rest.

@paf31
Copy link
Contributor

paf31 commented Nov 22, 2017

Thanks! I think a comment is enough for now.

@paf31
Copy link
Contributor

paf31 commented Nov 22, 2017

The alternative is to use a Warn constraint.

@matthewleon
Copy link
Contributor Author

Looks like I've made a test fail now. Will look into this.

@matthewleon
Copy link
Contributor Author

That test passes on my machine. Any idea what's going on there?

@paf31
Copy link
Contributor

paf31 commented Nov 23, 2017

It appears there is actually a bug:

> mapWithIndex (\x ix -> x + ix) (fromFoldable [0, 1, 2, 3])
(3 : 3 : 3 : 3 : Nil)

paf31
paf31 reviewed Nov 23, 2017
View reviewed changes
src/Data/List/Lazy/Types.purs Outdated
mapWithIndex f xs = List (go 0 <$> unwrap xs)
where
go _ Nil = Nil
go i (Cons x xs') = Cons (f i x) (f (i + 1) <$> xs')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be a recursive call to go here?

Maybe it's better to try to write this as a foldr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, indeed. I think I was running my tests locally on the wrong branch by accident.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think this is a different mistake I've made, besides the one breaking the test above. I'll go through and add some more thorough testing wherever it makes sense.

@matthewleon
Copy link
Contributor Author

matthewleon commented Nov 24, 2017 •
edited
Loading

Okay, this should be good to go now.

(That is to say, ready for further review.)

paf31
paf31 reviewed Nov 24, 2017
View reviewed changes
src/Data/List/Lazy/Types.purs
@@ -120,6 +125,22 @@ instance foldableList :: Foldable List where

foldMap f = foldl (\b a -> b <> f a) mempty

instance foldableWithIndexList :: FoldableWithIndex Int List where
foldrWithIndex f b xs =
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like we could extract a default implementation in terms of foldlWithIndex. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try.

@matthewleon
Copy link
Contributor Author

matthewleon commented Nov 25, 2017 •
edited
Loading

I don't think we can build a polymorphic default foldrWithIndex in terms of foldlWithIndex, actually. To begin, I don't think we can do a stack-safe default foldr in terms of foldl.

My reasoning:

We know foldr f a xs = foldl (flip f) a (reverse xs).

But this depends on a polymorphic reverse.

There's one here for Traversables: https://wiki.haskell.org/Foldable_and_Traversable

reverseT :: (Traversable t) => t a -> t a
reverseT t = snd (mapAccumR (\ (x:xs) _ -> (xs, x)) (toList t) t)

But this, in turn, depends on a toList, and our library dependencies go in the other direction.

Now, we do have a default implementation of foldr in terms of foldMap, which in turn is implemented in terms of foldl. But this uses the Endo monoid to build a thunk while traversing the Foldable, which kills our stack safety.

Please let me know if you think of something I've missed here. I would really love to get an efficient, stack-safe default implementation into foldable-traversable. But I can't think of any trick to do so.

@paf31
Copy link
Contributor

paf31 commented Nov 29, 2017

Looks good, and I think you're right about the default implementations.

Could you please add a test for each of the new methods though?

@matthewleon
Copy link
Contributor Author

matthewleon commented Nov 29, 2017 via email

@matthewleon
Copy link
Contributor Author

tests done

paf31
paf31 reviewed Nov 30, 2017
View reviewed changes
src/Data/List/Types.purs Outdated
@@ -91,6 +110,11 @@ instance traversableList :: Traversable List where
traverse f = map (foldl (flip (:)) Nil) <<< foldl (\acc -> lift2 (flip (:)) acc <<< f) (pure Nil)
sequence = traverse id

instance traversableWithIndexList :: TraversableWithIndex Int List where
traverseWithIndex f =
map (foldl (flip (:)) Nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this reverse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we can't use it because of the reverse dependency.

paf31
paf31 reviewed Nov 30, 2017
View reviewed changes
test/Test/Data/List.purs
assert $ traverseWithIndex (const Just) xs == Just xs

log "traverseWithIndex should be correct"
assert $ traverseWithIndex (\i a -> Just $ i + a) (fromFoldable [2, 2, 2])
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking, sorry: could you use an Applicative which tests that the order of effects is correct, like Array?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, actually that wouldn't work, never mind.

@paf31 paf31 merged commit ab3189f into purescript:master Nov 30, 2017
@paf31
Copy link
Contributor

paf31 commented Nov 30, 2017

Thanks!

matthewleon added a commit to matthewleon/purescript-lists that referenced this pull request Dec 19, 2017
@matthewleon
indexed foldable-traversable instances (purescript#134)
0f7ae00 
* indexed foldable-traversable instances

* update foldable-traversable dep

* TraversableWithIndex implementation

* Data.List: reuse mapWithIndex from Data.FunctorWithIndex

* fix List.foldrWithIndex

in turn fixes List.mapWithIndex

* simplify List right fold code

reuse left fold for reversals

* add failing mapWithIndex test for Lazy List

* fix Lazy List foldrWithIndex

* indexed instance tests

* clarify reversal in traversableWithIndexList
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paf31 paf31 paf31 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@matthewleon @paf31