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

Add sequtils.unzip to complement sequtils.zip #13429

Merged
merged 4 commits into from
Feb 20, 2020

Conversation

kaushalmodi
Copy link
Contributor

No description provided.

@def-
Copy link
Member

def- commented Feb 18, 2020

I would suggest newSeq(result[0], s.len) and then result[0][i] = s[i][0] etc. in order to only allocate memory once.

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Feb 18, 2020

@def- Thanks! @Vindaar's suggested commit in 980c0ea should take care of that.

Actually 2ff1e41 should take care of that.

@Vindaar
Copy link
Contributor

Vindaar commented Feb 19, 2020

@def- Thanks! @Vindaar's suggested commit in 980c0ea should take care of that.

Actually 2ff1e41 should take care of that.

Just FYI these two things should do essentially the same thing. newSeqOfCap simply allows one to use add on the sequence instead of having to access the i-th element, but it already allocates the space given as the argument.

@@ -281,6 +281,21 @@ when (NimMajor, NimMinor) <= (1, 0):
else:
zipImpl(s1, s2, seq[(S, T)])

proc unzip*[S, T](s: openArray[(S, T)]): (seq[S], seq[T]) =
Copy link
Member

Choose a reason for hiding this comment

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

Needs a .since annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kaushalmodi
Copy link
Contributor Author

@Vindaar Thanks I wasn't aware of that. I thought that the newSeqOfCap might be doing just a length check internally.

@kaushalmodi
Copy link
Contributor Author

@Vindaar Thanks I wasn't aware of that. I thought that the newSeqOfCap might be doing just a length check internally.

I have now reverted back to using newSeqOfCap as I like the .add syntax better.

@timotheecour
Copy link
Member

timotheecour commented Feb 20, 2020

Just FYI these two things should do essentially the same thing. newSeqOfCap simply allows one to use add on the sequence instead of having to access the i-th element, but it already allocates the space given as the argument.

[EDIT] not true, see #13448 (using []= can be 7X faster than add)

@Vindaar
Copy link
Contributor

Vindaar commented Feb 20, 2020

Just FYI these two things should do essentially the same thing. newSeqOfCap simply allows one to use add on the sequence instead of having to access the i-th element, but it already allocates the space given as the argument.

@Vindaar this is not true: as shown in #13448 (which fixes performance issue with unzip) newSeqOfCap(n) + add is 5-15X slower than newSeq(n) + []=

Wait, what? 😨

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.

5 participants