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: 936 embedding cell count #942

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

tihuan
Copy link
Contributor

@tihuan tihuan commented May 22, 2024

Fix #936

  1. doInitialDataLoad action now choose the initial layout by checking if the dataset's preferred default_embedding is available or not and fall back to picking the best layout given the available layouts
  2. This resolves two bugs:
    1. On page load, we no longer get the layout flickering caused by the layoutChoice first setting a fallback layout, and then switching to the dataset preferred layout
    2. Fixes BUG: Bug 1: Selected cells discrepancy between umap and spatial #936
  3. Extract a new selector selectAvailableLayouts to reuse
Screen.Recording.2024-05-22.at.9.08.36.AM.mov

@tihuan tihuan force-pushed the thuang-936-embedding-bug-1-from-main branch 2 times, most recently from 442d257 to 5411c52 Compare May 22, 2024 16:20
Comment on lines +246 to +254
dispatch({
type: "initial data load complete",
isCellGuideCxg,
layoutChoice: finalLayout,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we dispatch this action, we send both isCellGuideCxg and layoutChoice

* count when the page is first loaded.
* BUG: https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-explorer/936
*/
dispatch(embActions.layoutChoiceAction(finalLayout));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still dispatch this action in order to get the correct crossFilter, etc.

// eslint-disable-next-line @typescript-eslint/ban-types --- FIXME: disabled temporarily on migrate to TS.
class Embedding extends React.PureComponent<{}, EmbeddingState> {
// eslint-disable-next-line @typescript-eslint/ban-types --- FIXME: disabled temporarily on migrate to TS.
constructor(props: {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove unnecessary ctor per linter

this.state = {};
}

class Embedding extends React.PureComponent<Props> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just type cleanup in this file

return layouts[0];
}

function setToDefaultLayout(schema: Schema): LayoutChoiceState {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is now split out into selectAvailableLayouts and getCurrentLayout for reusability


// eslint-disable-next-line @typescript-eslint/no-explicit-any --- update typings once annoMatrix reducer state is typed.
export const selectAnnoMatrix = (state: RootState): any => state.annoMatrix;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type cleanup

@@ -20,3 +20,8 @@ export const selectIsUserStateDirty = (state: any): boolean => {
)
);
};

export function selectSchema(state: RootState) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new selector to get annoMatrix.schema

@tihuan tihuan requested a review from seve May 22, 2024 16:47
/**
* (thuang): This dispatch is necessary to ensure that we get the right cell
* count when the page is first loaded.
* BUG: https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-explorer/936
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* BUG: https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-explorer/936
* BUG: https://github.coms/chanzuckerberg/single-cell-explorer/936

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thank you! I'll use #936

@tihuan tihuan force-pushed the thuang-936-embedding-bug-1-from-main branch from 5411c52 to ace0a64 Compare May 23, 2024 21:40
@tihuan tihuan force-pushed the thuang-936-embedding-bug-1-from-main branch from ace0a64 to b6a7b39 Compare May 23, 2024 21:42
@tihuan tihuan merged commit 65d1281 into main May 23, 2024
4 of 5 checks passed
@tihuan tihuan deleted the thuang-936-embedding-bug-1-from-main branch May 23, 2024 21:59
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.

BUG: Bug 1: Selected cells discrepancy between umap and spatial
2 participants