TheJacksonLaboratory / LIRICAL

LIkelihood Ratio Interpretation of Clinical AbnormaLities
https://thejacksonlaboratory.github.io/LIRICAL/stable
Other
22 stars 11 forks source link

LRs in graphs for each disease are sorted by descending LR, sparkline LRs are not, and diamond icons are missing from sparkline #473

Closed justaddcoffee closed 4 years ago

justaddcoffee commented 4 years ago

Possibly not a big deal. To reproduce: java -jar target/LIRICAL.jar phenopacket -p test.txt with this phenopacket.

Example:

Screen Shot 2019-11-13 at 12 46 43 PM Screen Shot 2019-11-13 at 12 46 02 PM
pnrobinson commented 4 years ago

I fixed the sorting issue. I will take a look at the diamonds

pnrobinson commented 4 years ago

added a diamond to the sparkline. Made it a tad smaller, looks OK I hope

justaddcoffee commented 4 years ago

I'm seeing a little weirdness here - how come there is now only 1 grey diamond in the sparkline graph for Treacher Collins-Franceschetti Syndrome but 2 diamonds in the explanation of TCF down below?

Screen Shot 2019-11-14 at 9 39 16 AM Screen Shot 2019-11-14 at 9 39 04 AM

pnrobinson commented 4 years ago

crap I forgot the excludeds..should be working now

justaddcoffee commented 4 years ago

When I run this example phenopacket, I'm still seeing only 1 diamond in the sparkline, and two down below...

pnrobinson commented 4 years ago

@justaddcoffee I think this has now been addressed, can we close?

justaddcoffee commented 4 years ago

Sorry, I'm still seeing three bits of weirdness here. Could you confirm that you see the issues below on your end?

Example: When I run the phenopacket attached above: java -jar target/LIRICAL.jar phenopacket -p test.txt with 996592286d15d7d67491eb5c193c053897b3230d and look at the data for Fraser Syndrome (rank 3), I see these problems:

First, I think the sparkline graph and possibly the disease explanation graphs below still aren't sorted correctly. Up top in the sparkline, the bars are not sorted in descending order for Fraser. The phenotypes for Fraser Syndrome seem instead to be sorted in descending order of LR for Treacher Collins-Franceschetti Syndrome, the rank 1) disease:

Screen Shot 2019-11-19 at 8 50 08 AM

That is, phenotypes for all sparkline graphs are sorted by their LR for Treacher Collins-Franceschetti Syndrome, not the disease in question.

That's okay if that's what we want, seems like that's probably not what we want though.

Second, down below in the main graph of LR for Fraser, the LR are not sorted by descending LR:

Screen Shot 2019-11-19 at 9 04 47 AM

Third, the phenotypes and LR in the tooltip seem to be for the wrong bars.(The tooltips though look great, btw.) Example: the sixth LR bar is labeled "Downslanted palpebral fissures [HP:0000494]" to the right, but the tooltip says "XA: Delayed speech and language development[HP:0000750]":

Screen Shot 2019-11-19 at 9 12 58 AM

pnrobinson commented 4 years ago

@justaddcoffee Currently I am sorting all of the sparklines the same so that users can compare the profiles -- I thought that was better. Is it confusing? The other issues are definitely bugs! I am trying to trakck down a NaN in one of the gene tool tips.

justaddcoffee commented 4 years ago

@justaddcoffee Currently I am sorting all of the sparklines the same so that users can compare the profiles -- I thought that was better. Is it confusing?

Seems possibly a little confusing (but you know the users better than I do). I was thinking that each sparkline graph and main graph for each disease should make the case for/against a diagnosis for that disease, without regard to how the phenotypes are arranged for any other disease.

pnrobinson commented 4 years ago

Well, with the tool tips it should hopefully be clear. I was thinking that it is good to have the same order so that users cabn see at a glance how much any one HPO term is contributing to the various differentials, I think this is occasionally important!

justaddcoffee commented 4 years ago

I was thinking that it is good to have the same order so that users cabn see at a glance how much any one HPO term is contributing to the various differentials, I think this is occasionally important!

Sure, your call Peter!

pnrobinson commented 4 years ago

I think the one remaining item on this issue is that the tool tips are not being sorted correctly. The classes are not completley coordinated. The TestRest returns observed and excluded phenotypes separately, but it now returns one combined list of phenotype explanations, which makes it difficult to sort. One easy solution would be to separate the phenotype explanations into two different lists. Going forward we could refactor such that a stream would be used to separate observed and excluded in the SVG classes etc., which would probably simplify things a lot without much loss of performance.

pnrobinson commented 4 years ago

@justaddcoffee I think I have fixed this. I should have done a PR but was not thinking and commited to develop. I could also simplify some of the code. Please close if there is not other error outstanding!

justaddcoffee commented 4 years ago

@pnrobinson looks good, looks like sparkline graphs are sorted according to descending LR in the #1 diagnosis, and tooltips now match the labels off to the right as they should +1