feat(collections): add slidingWindows #1191

ayame113 · 2021-08-30T21:14:38Z

Did you support importing from mod.ts as well as your functions file?
Have you made sure to allocate as little memory and do as little steps in your algorithm as possible?
Did you add/adapt JSDoc comments, add/adapt an example, made sure that the example uses the ts markdown tag and assertEquals to show the result?
Did you add/adapt your functions documentation in README.md?
Did you make sure not to mutate the arguments passe to your function?
Did you add tests ensuring no mutation, empty input and corner cases?
Are your types flat, meaning there is no unnecessary alias in them that makes your users "go to type definition" twice to understand what to pass?

LionC

Only minor comments, well done overall, thanks!

collections/windowed.ts

LionC · 2021-08-31T06:53:26Z

collections/windowed.ts

+  const length = Math.floor((array.length - (partial ? 1 : size)) / step + 1);
+
+  return Array.from(
+    { length },
+    (_, i) => array.slice(i * step, i * step + size),
+  );


I really like this - it is concise, understandable but also not trivial.

LionC · 2021-08-31T06:54:55Z

collections/windowed.ts

+  const length = Math.floor((array.length - (partial ? 1 : size)) / step + 1);
+
+  return Array.from(
+    { length },
+    (_, i) => array.slice(i * step, i * step + size),


Personal nitpick: When too many things happen on one line, it starts to become hard to read. You could definitely split some value (like the ternary) into their own intermediary const. That is really just a nitpick though, so don't bother if you do not agree :-)

For now, the result of deno fmt is within single line, so I'd like to leave it as it is.

LionC · 2021-08-31T06:56:57Z

collections/windowed_test.ts

+        // @ts-ignore: for test
+        windowed([1, 2, 3, 4, 5], "invalid");


I don't think we need to test for invalid TS calls - if we start doing that, you might as well check every parameter.

If we forgot to throw an error here, the length will be NaN and we will get the unexpected behavior of returning an empty array. I'd like to leave this test alone to make sure that an error is thrown when passing a non-numeric value.

collections/windowed_test.ts

LionC · 2021-08-31T06:58:49Z

collections/windowed_test.ts

@@ -0,0 +1,220 @@
+// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
+


Very thorough tests overall - thanks!

ayame113 · 2021-09-01T16:48:33Z

I found it faster to use the for statement and array.push than to use Array.from, so I modified the code accordingly.

Benchmark for arrays of length 10000:

before -> 1.107292060000005ms

return Array.from(
  { length },
  (_, i) => array.slice(i * step, i * step + size),
);

after -> 0.36173926999999984ms

const result = [];
for (let i = 0; i < length; i++) {
  result.push(array.slice(i * step, i * step + size));
}
return result;

ghost · 2021-09-01T17:19:05Z

collections/windowed.ts

+  /** length of the return array */
+  const length = Math.floor((array.length - (partial ? 1 : size)) / step + 1);
+
+  const result = [];


Suggested change

const result = [];

const result = new Array(length);

Proposed Thank you.
Sure, new Array(length) (0.33261830000000053ms) is a bit faster than array.push (0.36173926999999984ms).
However, according to this V8 blog post, it seems that V8 can optimize the code better with array.push than with new Array(length). I would like to use array.push to improve the performance of subsequent code that uses the return value of this function.

you are pushing arrays into array, primitive value optimization is impossible in this case (not to mention type hint does not make up for time you waste reallocing memory)

@devcat what @ayame113 is (I think correctly) getting at, is that when you build an Array using the constructor with a length value, you are saving some performance for reallocation when filling it, but that Array is and will forever be markes as HOLEY_, which prevents v8 from applying performance optimizations to further operations on it like .map().

I think this is actually really interesting here and in all the other functions. We have been doing new Array(length) whenever possible, but thinking about it now, that means that we improve the performance of our function a bit, but return an unoptimizable HOLEY_ Array to the user that will perform worse in their code. Specifically for bigger Arrays, I don't think that is desirable, as the reallocation impact is only O(log n) and only happens once, while the impact of the user carrying a HOLEY_ Array through their code happens ok every single further operation on it.

I plan to do a benchmark suite for the whole module as soons as we are API complete, we should investigate that further once that lands.

@LionC
Thanks for the explanation, that's what I wanted to say. 👍

kt3k · 2021-09-02T09:28:19Z

@ayame113 Thank you for your contribution! Nice work!

The implementation and tests looks good to me.

But we try to avoid using adjective name for public API in general. ref: #1098 Can you think of any alternative name for this function?

ayame113 · 2021-09-02T13:35:33Z

@kt3k

I found a discussion that took place when I introduced the windowed function to Kotlin.
Kotlin/KEEP#11

According to it, other names used in other languages or suggested are:

slideWindow
subLists (subArrays)
eachSlice
collate
partition

I like slideWindow. I changed it to use slideWindow for the time being, but other names are fine.

LionC · 2021-09-02T13:44:11Z

@kt3k

I found a discussion that took place when I introduced the windowed function to Kotlin.

Kotlin/KEEP#11

According to it, other names used in other languages or suggested are:

slideWindow

subLists (subArrays)

eachSlice

collate

partition

I like slideWindow.

Something with "sliding" is very good I think. But why only one "window" when it returns multiple?

What about slidingWindows?

ayame113 · 2021-09-02T13:59:51Z

Something with "sliding" is very good I think. But why only one "window" when it returns multiple?
What about slidingWindows?

Oops, I missed this comment before commit 7677fd2. I think slidingWindows is good too.
At first, I thought slide was good because sliding sounds like a noun, but I'm not sure because I'm not a native speaker.

LionC · 2021-09-03T12:51:39Z

I would vote strongly for slidingWindows over slideWindow - I get the "verb" idea, but trying to understand what I get when reading slideWindow is hard for a non-native speaker (or for someone who does not expect a verb). Either way, both would be fine and are mergeable IMO, @kt3k can you make a call?

kt3k · 2021-09-04T07:49:47Z

Thank you @ayame113 for researching the past discussion in kotlin community.

Among 2 candidates, I slightly prefer slidingWindows because it exactly describes the returned value. slideWindow sounds a little bit obscure about what the returned value is.

ayame113 · 2021-09-04T08:35:45Z

Among 2 candidates, I slightly prefer slidingWindows because it exactly describes the returned value. slideWindow sounds a little bit obscure about what the returned value is.

I updated to use the slidingWindows.

kt3k

@ayame113 Thank you for updating the name 🙏

LGTM. Nice work!

ayame113 added 5 commits August 31, 2021 06:10

feat(collections): add windowed

731d1f4

chore: delete unnecessary sentences

a128c00

fix document error

08a1440

add empty Array test

1db3504

add deault value document and fix JSDoc

d2b72eb

ayame113 marked this pull request as ready for review August 30, 2021 21:45

ayame113 added 3 commits August 31, 2021 06:48

add large size test

d27da41

remove unnecessary asserts

5f1ace0

rename argument name

75c702f

LionC mentioned this pull request Aug 31, 2021

[Umbrella] collections APIs #1173

Closed

44 tasks

LionC suggested changes Aug 31, 2021

View reviewed changes

ayame113 added 4 commits September 2, 2021 01:23

Merge branch 'denoland:main' into add-collection-windowed

09d2d92

use array.push instead of Array.from

35216d8

use Number.isInteger instead of Number.isNaN

75cf681

add tests for floats and infinity

4724875

ghost reviewed Sep 1, 2021

View reviewed changes

rename windowed to slideWindow

7677fd2

rename slideWindow to slidingWindows

ca68045

ayame113 changed the title ~~feat(collections): add windowed~~ feat(collections): add slidingWindows Sep 4, 2021

kt3k approved these changes Sep 6, 2021

View reviewed changes

kt3k merged commit 3efa178 into denoland:main Sep 6, 2021

ayame113 deleted the add-collection-windowed branch September 13, 2021 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(collections): add slidingWindows #1191

feat(collections): add slidingWindows #1191

ayame113 commented Aug 30, 2021 •

edited

Loading

LionC left a comment •

edited

Loading

LionC Aug 31, 2021

LionC Aug 31, 2021

ayame113 Sep 1, 2021

LionC Aug 31, 2021

ayame113 Sep 1, 2021

LionC Aug 31, 2021

ayame113 commented Sep 1, 2021

ghost Sep 1, 2021

ayame113 Sep 2, 2021 •

edited

Loading

ghost Sep 2, 2021 •

edited by ghost

Loading

LionC Sep 2, 2021

ayame113 Sep 2, 2021

kt3k commented Sep 2, 2021

ayame113 commented Sep 2, 2021 •

edited

Loading

LionC commented Sep 2, 2021

ayame113 commented Sep 2, 2021 •

edited

Loading

LionC commented Sep 3, 2021

kt3k commented Sep 4, 2021

ayame113 commented Sep 4, 2021

kt3k left a comment

		// @ts-ignore: for test
		windowed([1, 2, 3, 4, 5], "invalid");

		@@ -0,0 +1,220 @@
		// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.

feat(collections): add slidingWindows #1191

feat(collections): add slidingWindows #1191

Conversation

ayame113 commented Aug 30, 2021 • edited Loading

LionC left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayame113 commented Sep 1, 2021

Choose a reason for hiding this comment

ayame113 Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

ghost Sep 2, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kt3k commented Sep 2, 2021

ayame113 commented Sep 2, 2021 • edited Loading

LionC commented Sep 2, 2021

ayame113 commented Sep 2, 2021 • edited Loading

LionC commented Sep 3, 2021

kt3k commented Sep 4, 2021

ayame113 commented Sep 4, 2021

kt3k left a comment

Choose a reason for hiding this comment

ayame113 commented Aug 30, 2021 •

edited

Loading

LionC left a comment •

edited

Loading

ayame113 Sep 2, 2021 •

edited

Loading

ghost Sep 2, 2021 •

edited by ghost

Loading

ayame113 commented Sep 2, 2021 •

edited

Loading

ayame113 commented Sep 2, 2021 •

edited

Loading