Reimplement the tree / network #15
Conversation
There was a problem hiding this comment.
Looks good! I had to skim a bit because of wanting to finish before a meeting, but I definitely didn't see any obvious issues.
You might want to consider documenting the algorithm for drawing the tree -- either in a comment block in code for the related file or somewhere else -- while it's fresh in your mind. That way if anyone ever has to fix a bug in it, they can fully understand the intent from your documentation and not need to figure it out from the code. Also it sounds like yours is one of the few implementations out there in the wild, so you know, be the change!
| width={2 * size} | ||
| height={2 * size} | ||
| fill={color} | ||
| key={`node-${uuid()}`} |
There was a problem hiding this comment.
Oh, I don't think this is good. I don't have enough time to think really carefully, but the idea of a key in React is to help prevent re-renders when you have a list of many components (usually via .map to make components). If each has a unique key determined by its props, then React can re-use that info somehow for less re-renders if the list changes order but maintains the same stuff.
If the key here has uuid() being called, then every attempt to re-render would generate a new uuid, so there would not (I'm pretty sure) be any of the benefits React is trying to get, because there would never be constancy around the component keys. This might be able to shut up the key warning, but (once again, I'm pretty sure) it's a way to avoid the warning, not the underlying reason the warning is there.
|
|
||
| export const unrootedTree = (props: unrootedTreeProps) => { | ||
| const { chartHeight, chartWidth, chartMargin } = props; | ||
| /** Initialize state */ |
There was a problem hiding this comment.
Nit: seems like an unnecessary comment?
| setScaleDomainX([minX, maxX]); | ||
| setScaleDomainY([minY, maxY]); | ||
| setReady(true); | ||
| // setReady(true); |
| @@ -125,19 +133,19 @@ export const ForceGraphLegend = (props: ForceGraphLegendProps) => { | |||
| y1={tickBarStartY + glyphHeight / 2} | |||
| y2={tickBarStartY - glyphHeight / 2} | |||
| stroke="darkgray" | |||
| /> | |||
| <text | |||
| /> */} | |||
There was a problem hiding this comment.
It's usually considered not great ("code smell") to leave large swathes of commented-out code for "final" code. It's super common practice to comment-out chunks of code while you work, but normally you want to have that all gone by the time you merge. There's a few reasons for this, but it's not something you always have to follow. If you have a really good reason for keeping it, you might be right, but if it's to hold on to a previous version in case you want to fall back, that's what version control is for.
There was a problem hiding this comment.
(This also applies to small chunks, even just a single line of commented-out code. It can be hard to tell why it's still there unless you have a lot of context, so the future dev is kind of stuck scratching their head wondering if this is a message from the past, something that was forgotten to be used, or something that was forgotten to be deleted.)
| if (parentNode.children.length > 0) { | ||
| if (!parentNode.parent || parentNode === mrca) { | ||
| /* | ||
| Initialize root of the (sub)tree directly with starting values |
There was a problem hiding this comment.
Nit: left-side spacing got messed up on this comment
There was a problem hiding this comment.
Did you remember to run Prettier?
…#200) * Initialize GH Action * Whoops, missed putting in folder * Troubleshoot deploy * Build and Deploy workflow (#3) * Curiosity/satisfaction killed/brought back * Bring in code checkout * NPM ain't standard, right? * Bring in node/npm to action * lol, always get hit by apostrophes * Take a crack at install and build * Verify step failure stops job * Actually try deploying, wooo * Hopefully fix base path GitHub Pages issues * try try again, change that route * Tweak path for router, would be nice... * Tweak the ending slash * Final slash is back; more generic routes tho? * No longer hardcode base path anywhere * Remove all those troubleshooting console logs * Fiddling with secrets * More secrets fiddling * Think we got that secret locked up folks * CI/CD for per env deploys in place * Okay, one more fiddle, for the road * Wrap up, only build/deploy on change to main * There is but one way to the throne * Console.log for verification * Custom domain means changing base path (#4) * Ugly: verify GH Pages works as expected w/domain * bye work on base paths, nice knowing you * Remove vite build mode, no longer needed * Back to only building on main merge * Add JSON file for easy dev (#5) * Add JSON file as easiest way to dev * Two thumbs and loves comments * add logo * fix logo img src * Disable sampling bias if pathogen isn't SARS-CoV-2 * Fix scatterplot legend typo * Clade definition redesign (#9) * Make the drawer wider to fully cover right pane; remove backdrop * Case definition subcomponent of sample selection * Initial pass at restyling per Janeece's designs * Split selectors into separate subcomponents. Start styling work (WIP) * Make the case definition "clear" button work * Move case definition filtering to util, simplify corresponding reducer * Mostly layout tweaks * Bonus: progressive disclosure for clustering * restore backdrop * Close case def after action complete * Copy * Use autocomplete instead of select to enable 'clear' functionality * alignment * FAB -> button * Add Prettier (formatting) linting as CLI tool (#10) * npm command to run typescript * Add in prettier * Initial Prettier CLI run * clean up drawer div margins / const for easier maintenance * Design feedback follow up on case definition redesign implementation (#11) * restore backdrop * Close case def after action complete * Copy * Use autocomplete instead of select to enable 'clear' functionality * alignment * FAB -> button * Layout tweaks * Margins * Title * Fetch public json from URL (#13) * Add axios for (dev friendly) requests * Convert to HashRouter for GH Pages * Verify HashRouter is happy on GH Pages * GH Pages working, back to normal * WIP: Figure out URL we need to fetch * WIP: Handle schrodingers https * WIP: Handle happy path of data fetch * Handle error states for fetch * Update README to include fetch route process * The pain of merge conflicts * Please let this be the end of Prettier pain (#14) * minHeight (#16) * Add tree file title to upload main box & modal (#17) * Pull tree file on load, pipe through to upload modal * typography * also show on main landing page after upload * Banners (beta + fetch errors) (#18) * banners as header subcomponents * action to clear error * update errors * detritus * rename action and reducer * Add in staging banner for later * Bare bones json format check + error states (#19) * rename for consistency * bare bones json format check * Validate json files before trying to ingest * manage state for showing error and clearing the error + fake tree once a good one is uploaded * Error banner * misc dead code cleanup * Edge cases re: fetch error display * Title display on fetch success * Reimplement the tree / network (#15) * First pass at reimplementing deterministically with equal angle algo * Fix logo positioning (one-off) * Fix tip count traversal (swear words) * be a bit more generous in the way we look for division and location * update package lock * Attempt 2 * cruft cleanup * fix the damn bug * cleanup * restore coloring, draw links underneath * Handle polytomies * Readability * Move aux functions to utils where possible * Default value * Fix demo muts per transmission value * Version with mrca square, SoI marker, tooltips * comments and readability * rename * squares as (containing) samples of interest * fix tooltip * cleanup for readability * Massive prettier / lint commit (!?) * Cleanup defunct files * One more defunct file * naming is hard * Set up Plausible (#22) * Majority of bringing in Plausible * Failsafe to ensure only load Plausible 1x * Verification of Vite env var process * Troubleshooting env var weirdness * Troubleshoot some more * le sigh * 90% of the time, it's something silly, ya know? * Back to sanity around env vars and deploy * Remove fetch in README until Plausible resolves * Helper to determine if on Staging * Prettier * [EngExc] Add support for absolute imports * Dynamic banner for when on staging to warn user * Prettier * Fetch instructions back in README Co-authored-by: Vincent Selhorst-Jones <[email protected]> Co-authored-by: vincent-czi <[email protected]>
We'd previously implemented a d3 force graph to generate the layout for the unrooted subtree graph. While we were able to generate a very nice graph for some clades (<100 nodes), the parameterization needed to keep the branch lengths consistent(ish) meant that this failed for larger clades (and vice versa -- could optimize for large clade layout but sacrificed branch lengths).
Ended up reimplementing a version of the "equal angle algorithm" from Felsenstein's text book, largely from scratch (could not find clearly implemented examples in other codebases, and the algo description in the book is quite vague at best).
There's evidence of some remaining wonkiness (see chanzuckerberg#192 ) in some cases, but these don't appear to have any impact on accuracy of the tree and largely don't interfere with visibility so I've split this off into a separate ticket.
I've also created a few additional follow up tickets which should get done in the near-ish future ideally but are not blockers for launch:
This PR also has a couple of one-off bug fixes that I encountered along the way
state.mutsPerTransmissionMaxon demo loadNotes for @vincent-czi --
I'd ideally like to move a lot of the auxiliary functions out of
drawUnrootedTreeinto a utils file, but when I try to do so, I get lots of typescript yelling because it appears to no longer understand SVG in a non-component file (!?) -- any ideas why or how to get around this? Not strictly required, but would make things much more readable / tidy.Running the linter as
npm run lint-formattingchanged 15 files. I'll look through to make sure these didn't break anything, otherwise fine leaving them but fyi it looks like there's a lot more changes than there are.