Skip to content

Conversation

Masynchin
Copy link
Contributor

Did it in Github Edit, will wait for CI for correctness. I would also update deleteAt, but it requires mapAccumulateFilter, which I did implemented but not finished in in typelevel/cats#4561

@Masynchin
Copy link
Contributor Author

@lenguyenthanh can you benchmark this changes when test pipeline becomes successful? I suppose it should be faster, because it doesn't use reverse, and, for List specialization of mapAccumulate, uses mutable variable for state rather then accumulator and uses map for transforming, rather then manual list construction (either :: with reverse or :+ which is O(n) (I suppose?))

@Masynchin
Copy link
Contributor Author

I may also delete Some(_) in modifyAt, since it uses only Some(true) and false, but not Some(false)

@ornicar
Copy link
Collaborator

ornicar commented Jun 2, 2025

Nice! I requested a CI re-run and it passed

@lenguyenthanh
Copy link
Member

lenguyenthanh commented Jun 2, 2025

@lenguyenthanh can you benchmark this changes when test pipeline becomes successful?

I don't think we have benchmark for this part of code base yet :( but I'll figure something out.

@lenguyenthanh
Copy link
Member

I suppose it should be faster, because it doesn't use reverse, and, for List specialization of mapAccumulate, uses mutable variable for state rather then accumulator and uses map for transforming, rather then manual list construction (either :: with reverse or :+ which is O(n) (I suppose?))

after checking cats implementation, I believe this too (also a bit lazy to write benchmark for it)

@lenguyenthanh lenguyenthanh merged commit bf05e49 into lichess-org:master Jun 2, 2025
5 of 6 checks passed
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.

3 participants