Skip to content

Conversation

dannon
Copy link
Member

@dannon dannon commented Mar 18, 2025

This provides the main visualization component for resolving #289. Separated color scheme options/discussion out per discussion on the call, which will be another PR.

There are several follow-ups to pursue but this is in a shape we can merge and move forward, with comments addressed.

image

@dannon dannon added the wip Work in progress label Mar 18, 2025
@dannon dannon changed the title Homepage sunburst display of available data feat: homepage sunburst display of available data Mar 19, 2025
@dannon dannon force-pushed the genome-viz-display branch 2 times, most recently from 951ba28 to 93c5e77 Compare March 24, 2025 17:31
@dannon dannon marked this pull request as ready for review March 25, 2025 17:23
@dannon dannon changed the title feat: homepage sunburst display of available data feat: homepage sunburst display of available data (#289) Mar 25, 2025
@dannon dannon force-pushed the genome-viz-display branch from 8e8ac9a to f4caefc Compare March 25, 2025 20:00
Copy link
Collaborator

@hunterckx hunterckx left a comment

Choose a reason for hiding this comment

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

I am told that this is about ready for review, so here are some potential issues I notice currently:

  • The mapping from rank to filter key still includes superkingdom, which should be replaced with domain and realm.
  • strain is also absent from the mapping, despite appearing in the tree.
  • It seems odd to me that the filter defaults to genus when the rank isn't in the mapping -- I would expect createFilterUrl to return null in that case maybe?
  • The leaf node counts that are labeled as "assemblies" in the interface don't really correspond to anything in the data browser (meaning the result count doesn't match when you open the link to the filtered assemblies) -- the assembly list in the data browser has multiple assemblies for some taxa, and the organisms list wouldn't fit either because it's a list of species (as opposed to potentially being more granular with strain/etc like the tree is).
    • I'm not sure what the best immediate solution would be, but something I've talked about with @NoopDog that I'm not sure ever got brought up elsewhere is that we could build the taxa tree from the taxonomic data that we were already using in the assembly list; if we did that it would likely be straightforward to count the assemblies as the tree is built.

Regarding the type errors in sunburst.tsx, I think there are three broad categories of errors that I see (although note that my impressions are based in large part on the type definitions for d3 so it's possible I'm missing some subtleties):

  • Lack of type annotations, both for parameters in function definitions and as type parameters to function calls; e.g.:
    • On line 42, getNodeColor lacks type annotations for its parameters, which causes them to default to any and is disallowed by our configuration; the d3 types suggest that the parameters should be typed as d: d3.HierarchyRectangularNode<SourceTreeNode>, root: d3.HierarchyRectangularNode<SourceTreeNode>, if the TreeNode type from data.ts was imported as SourceTreeNode.
    • On line 107, d3.partition() takes a type parameter that defaults to unknown, which causes a mismatch when hierarchyData is passed in later. The d3 types suggest that that initial function call should be d3.partition<SourceTreeNode>().
  • Inaccurate type annotations? It seems to me like there might be some confusion between the TreeNode type from data.ts, the TreeNode type from NodeDetails.tsx, and the d3 node types -- though looking more closely maybe TreeNode from NodeDetails is intentionally used for d3 nodes? (In which case I might expect it to be defined by extending a d3 type if possible.)
  • The addition of properties such as current to d3 nodes (assuming I'm understanding correctly and that's what's going on?) -- I think it would work better with TypeScript if that data could be stored outside the d3 node (such as via a map and/or the data associated with the node), but if that's not feasible a solution might be to define something like getCurrent and setCurrent functions that use type assertions to allow the current property to be referenced. Or, if the TreeNode type represents the addition of these properties, maybe just inline as TreeNode would be appropriate?

@dannon dannon force-pushed the genome-viz-display branch from 604f34d to f342fc3 Compare April 13, 2025 02:20
@dannon dannon force-pushed the genome-viz-display branch from efb9c1b to d56fe23 Compare April 21, 2025 13:02
@dannon dannon force-pushed the genome-viz-display branch 2 times, most recently from c668ceb to 315b63d Compare April 28, 2025 15:03
dannon and others added 22 commits April 28, 2025 11:18
drop a @ts-nocheck on this file until I can untangle all the d3 types.
…erit from there) to provide more contrast in species.
@dannon dannon force-pushed the genome-viz-display branch from 753db07 to 9c1d066 Compare April 28, 2025 15:18
@dannon dannon added feat and removed wip Work in progress labels Apr 28, 2025
@mvdbeek mvdbeek merged commit 3dc5dd1 into main Apr 28, 2025
2 checks passed
@mvdbeek mvdbeek deleted the genome-viz-display branch April 28, 2025 16:55
@mvdbeek
Copy link
Member

mvdbeek commented Apr 28, 2025

Awesome, works well @dannon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants