-
Notifications
You must be signed in to change notification settings - Fork 27
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
Opacity of tracks based on a score/mappability #460
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to update src/Types.ts
with this change before merging.
It looks like src/components/TrackPickerDisplay.test.js
also needs to be uodated to have the new field in the expected output in order for the tests to pass:
sequenceTubeMap/src/components/TrackPickerDisplay.test.js
Lines 173 to 177 in 0452ecb
trackColorSettings: { | |
mainPalette: "blues", | |
auxPalette: "reds", | |
colorReadsByMappingQuality: false, | |
}, |
It would be nice to take the opportunity to somehow remove the color by MAPQ flag and replace it with something less special and more unified with the palette system. But we don't necessarily need that for this.
colorReadsByMappingQuality: false, | ||
alphaReadsByMappingQuality: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're adding a new key to the color settings type we should update the schema we have here:
Line 85 in 0452ecb
colorReadsByMappingQuality: boolean |
@@ -12,7 +12,8 @@ import ColorPicker from "./ColorPicker"; | |||
* trackColorSettings expects an object in the form of | |||
* {mainPalette: string, | |||
* auxPallate: string, | |||
* colorReadsByMappingQuality: boolean} | |||
* colorReadsByMappingQuality: boolean, | |||
* alphaReadsByMappingQuality: boolean} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should cross-reference the schema file https://github.com/vgteam/sequenceTubeMap/blob/0452ecb82d057372e359a9b456d789336e5ab8a1/src/Types.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's now two flags, which is a suspicious number of flags. We should think about redesigning this soon.
Really, the new alpha flag is the only one that ought to be a real flag, since it can be on or off and the main and aux palettes still matter.
If you turn on the color by mapping quality flag for a read track, the main and aux palettes are ignored. But keeping them around makes it easier to turn on color-by-MAPQ and then turn it off again and get back the same colors you had set.
This was an idea to make it easier to distinguish separate tracks (with different assigned color palettes) and still visualize the mapq information by making the reads more transparent the lower the mapq. Right now, if we color by mapping quality, all the tracks get colored and we lose which path belong to which track.
I'm also using this option for "coverage" tracks to represent coverage level: the more opaque the higher the coverage.
Just a suggestion at this point. This was easy to add. Another harder option could be to provide a way for the user to define color palettes and/or color gradients for mapping quality.