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

Fix #104 dataframe histogram memoization #116

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Fix #104 dataframe histogram memoization #116

merged 1 commit into from
Oct 6, 2021

Conversation

blrnw3
Copy link
Contributor

@blrnw3 blrnw3 commented Oct 6, 2021

Memo was being set up on every call rather than once per instance, causing some UI features to be slow
e.g. https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/chanzuckerberg/single-cell-explorer/104.

These were the only instances of memo not working correctly that I could find. Also, this issue does not affect cellxgene desktop as the bug was introduced during the TS migration.

Perf screenshots when clicking a CatVar drop down (following the steps to reproduce above).
Before
Screen Shot 2021-10-06 at 9 55 37 AM

After
Screen Shot 2021-10-06 at 9 53 51 AM

Reviewers

Functional:
Bruce
Readability:
Seve

Changes

  • Fix memoization for dataframe's histogram functions

Memo was being set up on every call rather than once per instance
@blrnw3 blrnw3 linked an issue Oct 6, 2021 that may be closed by this pull request
@blrnw3 blrnw3 requested review from bkmartinjr and seve October 6, 2021 17:14
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.

Nice fix, which looks good. And embarrassing given this was my bug!

It is very likely the cellxgene repo has the same issue. Can you apply the fix to both?

@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #116 (1de5663) into main (d41d92b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #116   +/-   ##
=======================================
  Coverage   71.83%   71.83%           
=======================================
  Files         126      126           
  Lines       10106    10106           
=======================================
  Hits         7260     7260           
  Misses       2846     2846           
Flag Coverage Δ
frontend 71.83% <ø> (ø)
javascript 71.83% <ø> (ø)
unitTest 71.83% <ø> (ø)

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 d41d92b...1de5663. Read the comment docs.

@blrnw3
Copy link
Contributor Author

blrnw3 commented Oct 6, 2021

It is very likely the cellxgene repo has the same issue. Can you apply the fix to both?

@bkmartinjr It looks like cellxgene is not affected because the bug was introduced during the TS migration, which seems to have only been done on the hosted code. Is there a plan to convert cellxgene to TS?

@bkmartinjr
Copy link
Contributor

Is there a plan to convert cellxgene to TS?

No, there is not. Thanks for checking!

@blrnw3 blrnw3 merged commit b0360b8 into main Oct 6, 2021
@blrnw3 blrnw3 deleted the brodgers/104/memo branch October 6, 2021 20:36
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.

Categorical variable slow to expand
2 participants