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

PileupRenderer refactor #3734

Merged
merged 2 commits into from
May 31, 2023
Merged

PileupRenderer refactor #3734

merged 2 commits into from
May 31, 2023

Conversation

cmdcolin
Copy link
Collaborator

This "modularizes" the PileupRenderer into a variety of separate files to help control the sprawling nature of the single PileupRenderer class.

The 'coloring' methods were not actually accessing any of the class state so this PR proposes modularizing them into accessory files.

An alternative approach to this could be making a 'glyph' classes. It's tricky because there are a couple 'layers' for drawing a pileup glyph

  1. choosing color, affected by color by
  2. rendering the glyph in that color
  3. rendering the 'markup' to render over the glyph (which can be mismatches, modifications, per-base quality scores, or per-base lettering)

the 'functional' style of these helper functions is i think not a bad system and may even be better than object oriented style glyph code

Note: the functional style also can lend itself to lazy loading, this isn't done in this PR but could be added later too

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label May 30, 2023
@cmdcolin cmdcolin added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels May 30, 2023
@cmdcolin cmdcolin force-pushed the pileup_renderer_refactor branch 4 times, most recently from 9777595 to aac9446 Compare May 30, 2023 18:22
@cmdcolin cmdcolin force-pushed the pileup_renderer_refactor branch from aac9446 to ab283f7 Compare May 30, 2023 18:57
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #3734 (5ad5aa5) into main (527b08e) will increase coverage by 0.01%.
The diff coverage is 61.09%.

@@            Coverage Diff             @@
##             main    #3734      +/-   ##
==========================================
+ Coverage   64.37%   64.39%   +0.01%     
==========================================
  Files         962      977      +15     
  Lines       29958    29932      -26     
  Branches     7185     7176       -9     
==========================================
- Hits        19286    19275      -11     
+ Misses      10506    10491      -15     
  Partials      166      166              
Impacted Files Coverage Δ
packages/app-core/src/ui/App/ViewMenu.tsx 83.33% <ø> (ø)
...ignments/src/PileupRenderer/renderModifications.ts 0.00% <0.00%> (ø)
...ments/src/PileupRenderer/renderPerBaseLettering.ts 0.00% <0.00%> (ø)
...gnments/src/PileupRenderer/renderPerBaseQuality.ts 0.00% <0.00%> (ø)
plugins/alignments/src/PileupRenderer/colorBy.ts 5.40% <5.40%> (ø)
...ments/src/PileupRenderer/getAlignmentShapeColor.ts 39.02% <39.02%> (ø)
...s/alignments/src/PileupRenderer/renderAlignment.ts 55.00% <55.00%> (ø)
...w/src/LinearGenomeView/components/MiniControls.tsx 83.33% <80.00%> (+27.08%) ⬆️
.../alignments/src/PileupRenderer/renderMismatches.ts 80.64% <80.64%> (ø)
...alignments/src/PileupRenderer/renderMethylation.ts 83.78% <83.78%> (ø)
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin merged commit c2454aa into main May 31, 2023
@cmdcolin cmdcolin deleted the pileup_renderer_refactor branch May 31, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant