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

Added zipWithLongIndex, mapWithLongIndex and traverseWithLongIndexM #4247

Merged
merged 4 commits into from
Jul 1, 2022

Conversation

nikololiahim
Copy link
Contributor

@nikololiahim nikololiahim commented Jun 21, 2022

Implements #4245.

@armanbilge
Copy link
Member

armanbilge commented Jun 21, 2022

Nice work!! We should also add some laws similar to these.

def mapWithIndexRef[A, B](fa: F[A], f: (A, Int) => B): IsEq[F[B]] = {
val lhs = F.mapWithIndex(fa)(f)
val rhs = F.traverse(fa)(a => State((s: Int) => (s + 1, f(a, s)))).runA(0).value
lhs <-> rhs
}
def traverseWithIndexMRef[G[_], A, B](fa: F[A], f: (A, Int) => G[B])(implicit G: Monad[G]): IsEq[G[F[B]]] = {
val lhs = F.traverseWithIndexM(fa)(f)
val rhs = F.traverse(fa)(a => StateT((s: Int) => G.map(f(a, s))(b => (s + 1, b)))).runA(0)
lhs <-> rhs
}
def zipWithIndexRef[A, B](fa: F[A], f: ((A, Int)) => B): IsEq[F[B]] = {
val lhs = F.map(F.zipWithIndex(fa))(f)
val rhs = F.map(F.mapWithIndex(fa)((a, i) => (a, i)))(f)
lhs <-> rhs
}


#4246 seems related, so I might as well close it here.

Great, but it is a bit more involved so I think a second PR based on this one would be the way to do it :)

@armanbilge armanbilge linked an issue Jun 21, 2022 that may be closed by this pull request
@armanbilge
Copy link
Member

The other thing probably worth doing is adding a method like this one:

def mapWithIndexFromStrictFunctor[F[_], A, B](fa: F[A], f: (A, Int) => B)(implicit ev: Functor[F]): F[B] = {
var idx = 0
ev.map(fa) { a =>
val b = f(a, idx)
idx += 1
b
}
}

And using it to add a higher-performant override for these instances:
https://github.com/typelevel/cats/search?q=mapWithIndexFromStrictFunctor

@nikololiahim
Copy link
Contributor Author

@armanbilge tried to address the reviews in c1f9ed6

/**
* Same as [[traverseWithIndexM]] but the index type is [[Long]] instead of [[Int]].
*/
def traverseWithLongIndexM[G[_], A, B](fa: F[A])(f: (A, Long) => G[B])(implicit G: Monad[G]): G[F[B]] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem like this should require Monad.

I think I see why this implementation does (I think due to using State).

Copy link
Member

Choose a reason for hiding this comment

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

@johnynek the scaladoc for the Int version explains:

  /**
   * Akin to [[traverse]], but also provides the value's index in
   * structure F when calling the function.
   *
   * This performs the traversal in a single pass but requires that
   * effect G is monadic. An applicative traversal can be performed in
   * two passes using [[zipWithIndex]] followed by [[traverse]].
   */

@@ -60,4 +60,14 @@ private[cats] object StaticMethods {
}
}

def mapWithLongIndexFromStrictFunctor[F[_], A, B](fa: F[A], f: (A, Long) => B)(implicit ev: Functor[F]): F[B] = {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this! The next step is to use it to provide overrides of mapWithLongIndex in all the same places that the Int-version is used.
https://github.com/typelevel/cats/search?q=mapWithIndexFromStrictFunctor

Copy link
Contributor Author

@nikololiahim nikololiahim Jun 21, 2022

Choose a reason for hiding this comment

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

Should be done in 6fe035f. I don't know if I should update the tests too, or these implementations are used automatically somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure of the question :) Actually, the behavior of the implementations should be completely identical, and the tests should be verifying that. Nothing "automatic" going on AFAIK :)

@armanbilge armanbilge added this to the 2.9.0 milestone Jun 21, 2022
@nikololiahim nikololiahim force-pushed the traverseWithLongIndexM branch from 71eb69b to 6fe035f Compare June 21, 2022 18:49
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for picking this up :)

@armanbilge armanbilge requested a review from johnynek June 22, 2022 17:59
@nikololiahim nikololiahim changed the title added zipWithLongIndex, mapWithLongIndex and traverseWithLongIndexM Added zipWithLongIndex, mapWithLongIndex and traverseWithLongIndexM Jun 22, 2022
@armanbilge armanbilge merged commit 609a4f1 into typelevel:main Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add zipWithLongIndex and friends to Traverse
3 participants