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

Allow adding pre built numberic dicterm cluster #2580

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

congyu-lu
Copy link
Contributor

@congyu-lu congyu-lu commented Jan 9, 2025

Description

Please test with sjpp branch: ALL_prebuilt_matrix_numericDictTermCluster

Checklist

Check each task that has been performed or verified to be not applicable.

  • Tests: added and/or passed unit and integration tests, or N/A
  • Todos: commented or documented, or N/A
  • Notable Changes: updated release.txt, prefixed a commit message with "fix:" or "feat:", added to an internal tracking document, or N/A

called in mds3.init
*/
export async function mayInitiateNumericDictionaryTermplots(ds) {
if (!ds.cohort.numericDictTermClusterPlots) return
Copy link
Collaborator

Choose a reason for hiding this comment

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

please define type def for ds.cohort.numericDictTermClusterPlots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks just added

@xzhou82 xzhou82 force-pushed the allow_adding_preBuilt_numbericDictermCluster branch from f419fdf to 6f3918b Compare January 9, 2025 23:23
@xzhou82
Copy link
Collaborator

xzhou82 commented Jan 9, 2025

what do u think about combining the existing ds.cohort.termdb.numericDictTermCluster{} into the new property, thus gather related things in one place

ds.cohort.numericDictTermCluster{
  appName
  settings{}
  exclude
  plots[]
}

@congyu-lu
Copy link
Contributor Author

what do u think about combining the existing ds.cohort.termdb.numericDictTermCluster{} into the new property, thus gather related things in one place

ds.cohort.numericDictTermCluster{
  appName
  settings{}
  exclude
  plots[]
}

thanks that's a good idea, just combined

Copy link
Contributor

@karishma-gangwani karishma-gangwani left a comment

Choose a reason for hiding this comment

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

works for me. so list samples on spanning on matrix doesn't show values in second column. is that intentional or a bug?

@xzhou82
Copy link
Collaborator

xzhou82 commented Jan 10, 2025

did u commit? i don't see the two are combined

@congyu-lu
Copy link
Contributor Author

did u commit? i don't see the two are combined

sorry my git push failed, just pushed again, please check

settings?: {
[key: string]: any
}
getConfig?: (f: any) => void
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's purpose of getConfig()?

@xzhou82
Copy link
Collaborator

xzhou82 commented Jan 10, 2025

thanks a lot this looks better. please update sjpp branch in order to test

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