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 for incorrect tick format in matrix & update disco scale #2344

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

creilly8
Copy link
Contributor

@creilly8 creilly8 commented Nov 5, 2024

Description

Addresses the ticks in this matrix example appearing in scientific notation. Several new units were added to verify the correct rendering for varying data range conditions.

Also addresses request to update the color direction and tick labels in the disco plot. Please use this example to test.

Checklist

Check each task that has been performed or verified to be not applicable.

  • Tests: added and/or passed unit and integration tests, or N/A
  • Todos: commented or documented, or N/A
  • Notable Changes: updated release.txt, prefixed a commit message with "fix:" or "feat:", added to an internal tracking document, or N/A

@karishma-gangwani
Copy link
Contributor

karishma-gangwani commented Nov 5, 2024

Hi @creilly8 in mbmeta, genome browser tab, search 'MYC', will load cnv segments atop the protein tk, when I increment the color scale +1 one at a time under the fixed option, from -2/+2 to -3/+3 the UI doesn't do anything except change the number of ticks from 4 to 3. the scale still remains at -2/2 and changing it to -4/+4 changes the scale. and if I go all the way upto -8/+8 it comes down to -5/+5. Should it have some sort of a lower/upper limit based on the actual cnv?

@creilly8 creilly8 requested a review from aacic November 5, 2024 17:26
@creilly8
Copy link
Contributor Author

creilly8 commented Nov 5, 2024

Hi @creilly8 in mbmeta, genome browser tab, search 'MYC', will load cnv segments atop the protein tk, when I increment the color scale +1 one at a time under the fixed option, from -2/+2 to -3/+3 the UI doesn't do anything except change the number of ticks from 4 to 3. the scale still remains at -2/2 and changing it to -4/+4 changes the scale. and if I go all the way upto -8/+8 it comes down to -5/+5. Should it have some sort of a lower/upper limit based on the actual cnv?

Do you have a link?

@creilly8 creilly8 changed the title Fix for incorrect tick format in matrix Fix for incorrect tick format in matrix & update disco scale Nov 5, 2024
@karishma-gangwani
Copy link
Contributor

Hi @creilly8 in mbmeta, genome browser tab, search 'MYC', will load cnv segments atop the protein tk, when I increment the color scale +1 one at a time under the fixed option, from -2/+2 to -3/+3 the UI doesn't do anything except change the number of ticks from 4 to 3. the scale still remains at -2/2 and changing it to -4/+4 changes the scale. and if I go all the way upto -8/+8 it comes down to -5/+5. Should it have some sort of a lower/upper limit based on the actual cnv?

Do you have a link?

try this you will have to edit the scale and check though.

@creilly8
Copy link
Contributor Author

creilly8 commented Nov 5, 2024

Hi @creilly8 in mbmeta, genome browser tab, search 'MYC', will load cnv segments atop the protein tk, when I increment the color scale +1 one at a time under the fixed option, from -2/+2 to -3/+3 the UI doesn't do anything except change the number of ticks from 4 to 3. the scale still remains at -2/2 and changing it to -4/+4 changes the scale. and if I go all the way upto -8/+8 it comes down to -5/+5. Should it have some sort of a lower/upper limit based on the actual cnv?

Do you have a link?

@karishma-gangwani, I see the scale change the tick placement according to the overall range and the track reload. The placement of the ticks and the labels is dictated by d3. I'm not sure what you mean by lower and upper limit. If it's input validation, then the caller needs to validate the input. The color scale wouldn't know what would be the absolute max and min values for the track.

@karishma-gangwani
Copy link
Contributor

karishma-gangwani commented Nov 5, 2024

also, in the matrix link you shared the scale numbers seem shifted for the gain. can you check?

image

@creilly8
Copy link
Contributor Author

creilly8 commented Nov 5, 2024

also, in the matrix link you shared the scale numbers seem shifted for the gain. can you check?

image

The scale uses the actual max value instead of 1. That's why 1 appears to the left.

@karishma-gangwani
Copy link
Contributor

karishma-gangwani commented Nov 5, 2024

also in copy number loss, shouldn't the darker blue represent a higher magnitude of the loss? so the scale should be reverse for that one. -1 -2 representing heterozygous, homozygous deletion should be on the darker end and not the lighter
@xzhou82 can you confirm?

image

This is fixed in 7bf03cd.

@karishma-gangwani
Copy link
Contributor

also in copy number loss, shouldn't the darker blue represent a higher magnitude of the loss? so the scale should be reverse for that one. -1 -2 representing heterozygous, homozygous deletion should be on the darker end and not the lighter @xzhou82 can you confirm?

image

This is fixed in 7bf03cd.

Thanks! Looks good

if ((min <= 0.01 && min != 0 && minDec >= 2) || (max <= 0.01 && max != 0 && maxDec >= 2)) {
/**Tick values are sorted in niceNumLabels.
* If min or max value are small nums, have 2 or more decimal places,
* use scientific notation. Do not use if either value is 0. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not use if either value is 0 is not valid. e.g. range is from 0 to 100000. still want scientific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the cutoff? When the range between the min and max is >=10 use scientific notation? or 100? or 1000?

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

Successfully merging this pull request may close these issues.

3 participants