TS typing for Dataframe #2382

bkmartinjr · 2021-08-15T18:09:19Z

This PR includes:

TS typing for Dataframe
a few minor bug fixes turned up by static type analysis
Partial upstream typing of Dataframe users. (the PR was getting big, so I will finish the rest of the Dataframe-consumer typing in another PR).

Notable API changes:

summarize() and histogram() are no longer polymorphic and have separate functions for categorical, continuous and by-category (eg, summarizeContinuous(), summarizeCategorical(), etc).
LabelIndex.getOffset() and getOffsets() returns -1 instead of undefined if the label is unknown.

This is another partial fix for #2366

codecov · 2021-08-15T18:15:49Z

Codecov Report

Merging #2382 (54142b1) into main (3fdf5ca) will not change coverage.
The diff coverage is n/a.

❗ Current head 54142b1 differs from pull request most recent head ec89306. Consider uploading reports for the commit ec89306 to get more accurate results

@@           Coverage Diff           @@
##             main    #2382   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files         127      127           
  Lines       12259    12259           
=======================================
  Hits         8365     8365           
  Misses       3894     3894

Flag	Coverage Δ
frontend	`66.99% <ø> (ø)`
javascript	`66.99% <ø> (ø)`
smokeTest	`∅ <ø> (∅)`
smokeTestAnnotations	`∅ <ø> (?)`
unitTest	`66.99% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fdf5ca...ec89306. Read the comment docs.

tihuan

LGTM!! Thanks for the massive PR, Bruce 🤩 👏 !! This looks amazing!

Just a few nits I could find 😆 Thanks again!

tihuan · 2021-08-16T18:12:32Z

client/src/util/dataframe/dataframe.ts

@@ -141,17 +163,13 @@ class Dataframe {
    Object.freeze(this);
  }

-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
+  /** @internal */


Ayyy @internal 🔥 !!

tihuan · 2021-08-16T18:14:44Z

client/src/util/dataframe/dataframe.ts

-      return offset >= 0 && offset < length;
+    const has = function has(rlabel: LabelType) {
+      const offset = getRowOffset(rlabel);
+      return offset !== undefined && offset >= 0 && offset < length;


super nit: do we need to check offset !== undefined if getRowOffset() always returns a number?

good catch - that is a BUG!!!

Hurray! Do we need code change here? It seems to still be offset !== undefined && offset >= 0 && offset < 😆

Only because the comment refers to the outdated code. Take a look at the most recent commit to the PR, not the obsolete commit :-)

tihuan · 2021-08-16T18:16:25Z

client/src/util/dataframe/dataframe.ts

-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/no-explicit-any -- - FIXME: disabled temporarily on migrate to TS.
-  isubsetMask(rowMask: any, colMask = null, withRowIndex = null) {
+  isubsetMask(
+    rowMask: Uint8Array | boolean[] | null,


Nice!! Types are so helpful here 🤩 👏

tihuan · 2021-08-16T18:19:35Z

client/src/util/dataframe/labelIndex.ts

-    return Number.isInteger(i) && i >= 0 && i < this.maxOffset ? i : undefined;
+    return Number.isInteger(offset) && offset >= 0 && offset < this.maxOffset
+      ? offset
+      : undefined;


does it make sense to return -1 instead of undefined here to have consistency with getOffset() above ?

No, it doesn't. -1 is a legal label.

Remember:

offsets are numeric indices. -1 is "no such offset"

labels are any string|bool|number. undefined is "no such label"

This is documented in the comments for getOffset() and getLabel()

What makes this particular class confusing is that it is the identity label index (aka offset===label).

ooh got it! Thanks for the explanation 🎉 Makes sense to me now 😄 !

tihuan · 2021-08-16T18:20:43Z

client/src/util/dataframe/labelIndex.ts

-      // eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
-      return arr.map((i: any) => this.getLabel(i));
+    this.getLabel = function getLabel(offset: number) {
+      return Number.isInteger(offset) ? labels[offset] : undefined;


same question here for undefined vs. -1

same answer as above. See the abstract method definition.

tihuan · 2021-08-16T18:21:57Z

client/src/util/dataframe/labelIndex.ts

-      // eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
-      return arr.map((i: any) => this.getLabel(i));
+    this.getLabel = function getLabel(offset: number) {
+      return Number.isInteger(offset) ? rindex[offset] : undefined;


same question here for undefined vs. -1

same answer as above. See the abstract method definition.

tihuan · 2021-08-16T18:25:12Z

client/src/util/dataframe/types.ts

+}
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any --- a legitimate use of any.
+export type AnyFunction<T = unknown> = (...args: any[]) => T;


just curious if unknown[] would work here instead of any[]?

https://blog.logrocket.com/when-to-use-never-and-unknown-in-typescript-5e4d6c5799ad/

No, it won't work as you can't call the memoized function without a type error. See, for example, how TS defines the signatures to Function.call, Function.bind, etc.

What would be better is to improve the type pass through for the memoize() function, which is the only user of this type. I'll fix that for both memoize() and callOnceLazy().

Eg,

export function memoize< T extends (...args: any[]) => any = (...args: any[]) => any >( fn: T, hashFn: (...args: Parameters<T>) => string, maxResultsCached = -1 ): (...args: Parameters<T>) => ReturnType<T> { ... }

Side note: fixing this pointed out a small typing bug! Yay.

ps. if you haven't already done so, it is worth studying the type signatures for stuff like Function.bind, Function.apply, etc.

Ahh didn't think of checking out those signatures! Thanks for the tip 👍

Using Parameters<T> SGTM!

MillenniumFalconMechanic

LGTM, thanks Bruce!

This reverts commit 45cecad.

bkmartinjr added 3 commits August 12, 2021 13:12

initial TS typing

76cb702

first cut at Dataframe TS typing

025fe7c

Merge branch 'main' into bkmartinjr/2366-dataframe

9ff3740

bkmartinjr added 3 commits August 15, 2021 12:49

more Dataframe typing

51155f9

comments

565200f

more Dataframe cleanup

7324a26

bkmartinjr marked this pull request as ready for review August 15, 2021 23:29

bkmartinjr requested review from MillenniumFalconMechanic, tihuan and seve August 15, 2021 23:29

bkmartinjr self-assigned this Aug 16, 2021

tihuan approved these changes Aug 16, 2021

View reviewed changes

PR review fixes and improvements

ec89306

seve approved these changes Aug 16, 2021

View reviewed changes

bkmartinjr mentioned this pull request Aug 17, 2021

Added typings to annoMatrix dir. (#2365) #2371

Merged

MillenniumFalconMechanic approved these changes Aug 17, 2021

View reviewed changes

bkmartinjr merged commit 45cecad into main Aug 17, 2021

bkmartinjr deleted the bkmartinjr/2366-dataframe branch August 17, 2021 17:43

bkmartinjr pushed a commit that referenced this pull request Aug 19, 2021

Revert "TS typing for Dataframe (#2382)"

c3deb5f

This reverts commit 45cecad.

This was referenced Aug 19, 2021

TS revert #2400

Closed

TS Revert (1) #2402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TS typing for Dataframe #2382

TS typing for Dataframe #2382

bkmartinjr commented Aug 15, 2021 •

edited

Loading

codecov bot commented Aug 15, 2021 •

edited

Loading

tihuan left a comment

tihuan Aug 16, 2021

tihuan Aug 16, 2021

bkmartinjr Aug 16, 2021

tihuan Aug 16, 2021

bkmartinjr Aug 17, 2021

tihuan Aug 16, 2021

tihuan Aug 16, 2021

bkmartinjr Aug 16, 2021 •

edited

Loading

tihuan Aug 16, 2021

tihuan Aug 16, 2021

bkmartinjr Aug 16, 2021

tihuan Aug 16, 2021

bkmartinjr Aug 16, 2021

tihuan Aug 16, 2021

bkmartinjr Aug 16, 2021

bkmartinjr Aug 16, 2021

tihuan Aug 16, 2021

MillenniumFalconMechanic left a comment

TS typing for Dataframe #2382

TS typing for Dataframe #2382

Conversation

bkmartinjr commented Aug 15, 2021 • edited Loading

codecov bot commented Aug 15, 2021 • edited Loading

Codecov Report

tihuan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkmartinjr Aug 16, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MillenniumFalconMechanic left a comment

Choose a reason for hiding this comment

bkmartinjr commented Aug 15, 2021 •

edited

Loading

codecov bot commented Aug 15, 2021 •

edited

Loading

bkmartinjr Aug 16, 2021 •

edited

Loading