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 breakpoint split view visualizations for files that need ref renaming (e.g. chr1 vs 1) #1911

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Apr 15, 2021

Fixes #1902

This makes sure to use canonical refname when searching.

There are possible alternatives to this PR

We could try to make all our adapters return features with a field called feature.get('originalRefName') that can be used for purposes like this (note that adapters do receive a parameter called region.originalRefName which is the canonical ref name for the assembly being views to their getFeatures function)

This could either

a) be the adapters responsibility to fill out this field or
b) be some sort of auto-added thing by some other aspect of our system (renderer/serialization/adapter/something else?)

Or we stick with this this type of solution, and wait until we find a good abstraction

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 15, 2021
@cmdcolin cmdcolin changed the title Add ability to get canonical refnames from features Fix breakpoint split view visualizations for files that use alternative refnames Apr 15, 2021
@cmdcolin cmdcolin changed the title Fix breakpoint split view visualizations for files that use alternative refnames Fix breakpoint split view visualizations for files that need ref renaming (e.g. chr1 vs 1) Apr 15, 2021
@cmdcolin cmdcolin force-pushed the fix_refname_renamed_breakpoint_split_view branch from a866bea to c8e7625 Compare April 15, 2021 20:42
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 15, 2021
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #1911 (c8e7625) into main (bcf8f30) will increase coverage by 0.23%.
The diff coverage is 2.70%.

❗ Current head c8e7625 differs from pull request most recent head 106094e. Consider uploading reports for the commit 106094e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1911      +/-   ##
==========================================
+ Coverage   58.21%   58.45%   +0.23%     
==========================================
  Files         461      460       -1     
  Lines       21464    21411      -53     
  Branches     5095     5093       -2     
==========================================
+ Hits        12495    12515      +20     
+ Misses       8658     8589      -69     
+ Partials      311      307       -4     
Impacted Files Coverage Δ
packages/core/util/types/index.ts 61.53% <ø> (ø)
...split-view/src/components/AlignmentConnections.tsx 13.04% <0.00%> (-1.72%) ⬇️
...breakpoint-split-view/src/components/Breakends.tsx 11.94% <0.00%> (-3.16%) ⬇️
...point-split-view/src/components/Translocations.tsx 12.50% <0.00%> (-2.26%) ⬇️
...-split-view/src/components/BreakpointSplitView.tsx 66.66% <100.00%> (ø)
...ctorWidget/components/HierarchicalTrackSelector.js 56.25% <0.00%> (-3.36%) ⬇️
...roducts/jbrowse-desktop/src/sessionModelFactory.ts 1.70% <0.00%> (-2.38%) ⬇️
packages/core/ui/DrawerWidget.js 93.10% <0.00%> (-1.19%) ⬇️
...ments/src/SNPCoverageAdapter/SNPCoverageAdapter.ts 94.16% <0.00%> (-0.84%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcf8f30...106094e. Read the comment docs.

@cmdcolin cmdcolin force-pushed the fix_refname_renamed_breakpoint_split_view branch from c8e7625 to 106094e Compare April 16, 2021 15:03
@cmdcolin
Copy link
Collaborator Author

I tried out one of the alternatives mentioned e.g. adding a canonicalRefName field to a feature https://github.com/GMOD/jbrowse-components/tree/canonical_refname_field

While this is tempting alternative, it is not necessarily a complete solution because the view would still have to call assembly.getCanonicalRefName(feature.INFO.CHR2) for example on Translocations, e.g. it wouldn't have a canonical refname for the other side of a TRA feature

Since it isn't really a complete solution to have feature.get('canonicalRefName') I might suggest we stick with the type of solution proposed in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant