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

chore: type centroid util #36

Merged
merged 17 commits into from
Sep 15, 2021
Merged

chore: type centroid util #36

merged 17 commits into from
Sep 15, 2021

Conversation

seve
Copy link
Contributor

@seve seve commented Sep 2, 2021

Type the centroid util and call sites as well as needed imports.

Note:

I did some retyping of the CategeoricalColumnSchema to make the categories field required on columns with type `categorical'. We'll have to do some tuning (#35) to reduce casting in favor of type guarding.

I also had to untype nextSharedState (it was typing as RootState) as I believe it opens up a self reference . This should be fixed in #9 cc: @colinmegill

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #36 (7239682) into main (32d5f08) will not change coverage.
The diff coverage is n/a.

❗ Current head 7239682 differs from pull request most recent head 700517d. Consider uploading reports for the commit 700517d to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #36   +/-   ##
=======================================
  Coverage   71.70%   71.70%           
=======================================
  Files         126      126           
  Lines        9988     9988           
=======================================
  Hits         7162     7162           
  Misses       2826     2826           
Flag Coverage Δ
frontend 71.70% <ø> (ø)
javascript 71.70% <ø> (ø)
smokeTest ?
unitTest 71.70% <ø> (ø)

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 32d5f08...700517d. Read the comment docs.

@seve seve requested review from bkmartinjr and tihuan September 2, 2021 02:02
@seve seve marked this pull request as ready for review September 2, 2021 02:02
Copy link
Contributor

@tihuan tihuan 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 updating the types, @seve 🙏 Just one question! Thank you!

client/src/util/stateManager/schemaHelpers.ts Show resolved Hide resolved
Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

Overall looks good, with one caveat: the centrol helpers assume all labels are strings, which is incorrect AFAIK. We can have datasets with non-string categoricals (numbers, bools, etc).

@seve seve requested a review from bkmartinjr September 14, 2021 21:44
@@ -31,7 +31,11 @@ describe("centroid", () => {
schema,
"field4",
obsAnnotations,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: given the confusion around the type of an obsAnnnotation, you may want to add tests which exercise the other label types (eg, number or boolean or blended)

@bkmartinjr
Copy link
Contributor

@seve - you re-requested a review, but I don't see any changes to util/centroid.ts -- it still assumes that all obs categories contain only strings, as far as I can tell...

Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

LGTM.

You might consider additional test coverage as noted inline.

@seve seve merged commit 0721964 into main Sep 15, 2021
@seve seve deleted the seve/centroid-util branch September 15, 2021 18:16
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