update genotype display for multiple sample types #4437

jklugherz · 2024-10-16T20:41:54Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

hanars · 2024-10-16T21:55:12Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

        <Popup
          header="Additional Sample Type"
-          trigger={<Icon name="plus circle" color={hasConflictingNumAlt ? 'red' : 'green'} />}
+          trigger={<Icon name="plus circle" color={firstGenotype.hasConflictingNumAlt ? 'red' : 'green'} />}


can you add a screenshot for this

ui/shared/components/panel/variants/VariantIndividuals.jsx

hanars · 2024-10-21T15:25:13Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

        <Popup
          header="Additional Sample Type"
-          trigger={<Icon name="plus circle" color={hasConflictingNumAlt ? 'red' : 'green'} />}
+          trigger={<Icon name="plus circle" color={firstGenotype.hasConflictingNumAlt ? 'red' : 'green'} />}


ui/shared/components/panel/variants/VariantIndividuals.jsx

hanars · 2024-10-21T15:32:34Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

-  // Temporarily use the first genotype for an individual until blended es/gs are supported in UI - https://github.com/broadinstitute/seqr/issues/4269
-  genotype = Array.isArray(genotype) ? genotype[0] : genotype
-
+const getAllGenotypeDetails = (genotype, variant, individual, isCompoundHet, genesById) => {
  const hasCnCall = isCalled(genotype.cn)
  if (!hasCnCall && !isCalled(genotype.numAlt)) {
    return <b>NO CALL</b>


this will definitely break with the way this is abstracted into a helper

this is still a problem

oh I see why it's a problem now.. return <b>NO CALL</b> is supposed to be a return value of Genotypes

ui/shared/components/panel/variants/VariantIndividuals.jsx

hanars · 2024-10-21T15:47:15Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+    genotypeInfo => genotypeInfo.genotype.numAlt !== firstGenotype.genotype.numAlt,
+  )
+
+  return (


You should use the Alleles class here instead of recreating its behavior. While the prop type for warning in that calls is currently a string, changing that proptype definition to node with no other changes to the class itself and then creating a react node for the warning content here and passing that as the warning prop will work totally fine, as the warning prop is rendered directly in that class

hanars · 2024-10-22T21:16:55Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

 export const Alleles = React.memo(({ genotype, variant, isHemiX, warning }) => (
  <AlleleContainer>
    {warning && (
      <Popup
        wide
        trigger={<WarningIcon />}
-        content={


so as mentioned on a previous PR, this is an exported component so its better to its behavior alone. I also think it actually better for us to say "warning" one time in the popup, not once per warningas repeating the word multiple time reduces readability

hanars · 2024-10-22T21:17:31Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

-  // Temporarily use the first genotype for an individual until blended es/gs are supported in UI - https://github.com/broadinstitute/seqr/issues/4269
-  genotype = Array.isArray(genotype) ? genotype[0] : genotype
-
+const getAllGenotypeDetails = (genotype, variant, individual, isCompoundHet, genesById) => {
  const hasCnCall = isCalled(genotype.cn)
  if (!hasCnCall && !isCalled(genotype.numAlt)) {
    return <b>NO CALL</b>


this is still a problem

hanars · 2024-10-22T21:20:20Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+  variant: PropTypes.object,
+}
+
+const LegacyAdditionalSampleTypePopup = ({ genotype, variant, isHemiX, genesById }) => {


We should never be maintaining a special legacy UI unless there is an overwhelming reason not to use the new UI. You should be transforming the otherSample into an array of genotypes with both samples and using the new, supported UI

I added otherSample to the genotypes array, screenshot incoming.

otherSample is not part of any saved variant json, it is added to the genotype inside of the es_utils code path. Is this something I can test?

Hmm okay so not easy to test but we should keep support for it, but we do not need to keep the current UI for it. I think changing the code to move it into the the genotypes array is sufficient here, no screenshot needed

hanars · 2024-10-22T21:31:57Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+            </div>
+          ))}
+          {hasDifferentNumAlt && (
+            <span>


This is going to look weird if you have a sample-type specific warnings and also different number of genotypes - it will be something like

WES: Potential UPD/ Hemizygosity WGS: Potential UPD/ Hemizygosity. Variant absent in parents Genotypes differ across sample types WES: G/C WGS: C/C

I think the differing genotypes warning belongs at the top, and also that rather than combining all the warnings per sample type, we should show each warning on each own row, with an indicator if its only foor some sample types:

Genotypes differ across sample types WES: G/C WGS: C/C Potential UPD/ Hemizygosity Variant absent in parents (WGS)

I agree here!

hanars · 2024-10-22T21:37:25Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+                <div key={genotype.sampleType}>
+                  {genotype.sampleType}
+                  :
+                  <AlleleContainer>


you can just use the Alleles class here, as without a warning prop or an SV (which can't have multiple sample types) this is just the same code as that class

hanars · 2024-10-22T21:39:44Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+            warning={
+              <div>
+                <b>Warning: </b>
+                {getWarningsForGenotype(genotypes[0], variant, individual, isHemiX, isCompoundHet)}


As mentioned above warning formatting needs to be left the same in the Alleles class instead of moving it here. One key problem with this current approach is that even if getWarningsForGenotype returns nothing this will still show a warning icon on every genotype. Whenever I change the variant UI based on changes from search, I always make sure to also open up a saved variant page for an unchanged family, to make sure that legacy saved variants still look right

hanars · 2024-10-22T21:45:01Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+          />
+        </span>
+      ) : (
+        <MultiSampleTypeAlleles


So in the end MultiSampleTypeAlleles returns Alleles using the same props as for a single-sample genotype - the only difference is the warning. Rather than have a helper class and a ternary here, it would be much neater to have a getWarningsForGenotypes method that handles constructing the correct warning for all the genotypes and leave this rendering much simpler

I removed MultiSampleTypeAlleles and conditionally set the warning prop on Alleles based on genotypes length.

hanars · 2024-10-27T17:11:03Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+    (a, b) => SAMPLE_TYPE_DISPLAY_ORDER.indexOf(a.sampleType) - SAMPLE_TYPE_DISPLAY_ORDER.indexOf(b.sampleType),
+  )
+
+  if (!isCalled(genotypes[0].cn) && !isCalled(genotypes[0].numAlt)) {


nitpcik, but shouldn't you check that all the genotypes are not called for this, not just the first one

I was thinking that since cn is an SV-only annotation and there will only be one genotype per SV, there would only be one genotype in the list here but I could be wrong.

its true for now but if this ever changes its unlikely we would think to go back to this spot in the UI code and fix it. Building the UI to generically work with multiple sample types is better as it means we are unlikely to introduce bugs in the future, as long as it doesn't make the code to complex/ unreadable for now

that makes sense, I updated the code to return no call if all genotypes fail that check

ui/shared/components/panel/variants/VariantIndividuals.jsx

hanars · 2024-10-27T17:17:05Z

ui/shared/components/panel/variants/VariantIndividuals.jsx

+
+  const sampleTypeWarnings = genotypes.reduce((acc, genotype) => {
+    const warnings = getWarningsForGenotype(genotype, variant, individual, isHemiX, isCompoundHet)
+    warnings.forEach((warning) => {


this reduction is not used in the case where theres only one sample type, so it would be neater to just do a genotypes.map here to get an array of warnings. Then for the single sample case your warnings would just be genotypeWarnings[0].join('. ') and in getWarningsForGenotypes you could do this grouping,
something like

`genotypeWarnings.reduce((acc, [warnings, index]) => { const { sampleType } = genotypes[index] warnings.forEach((warning) => { ... }, {})

agree, done!

jklugherz added 3 commits October 16, 2024 16:40

update genotype display for multiple sample types

41a8bf7

resolve lint errors

352a6cf

remove es component

8f473b6

jklugherz marked this pull request as ready for review October 16, 2024 21:02

jklugherz requested review from hanars and bpblanken October 16, 2024 21:02

hanars reviewed Oct 16, 2024

View reviewed changes

jklugherz added 4 commits October 17, 2024 11:44

sample type in popup

f87efb4

sample type in popup

2a2efb2

new component

b89452f

key

67fd314

hanars requested changes Oct 21, 2024

View reviewed changes

jklugherz added 6 commits October 21, 2024 17:37

refactor

d5cb77c

empty line

fc086a7

fix diff

88e0cf3

fix diff?

3a5cc74

period!

a994a2b

Merge branch 'dev' into wes-wgs-ui

3bd74b9

hanars reviewed Oct 22, 2024

View reviewed changes

jklugherz added 7 commits October 23, 2024 16:28

comments

4590165

undo allelesheader

e1efd7b

only show sampletype if there is more than one

de8a7eb

minor things

e1bbeb3

warning div key id

d85e79a

move a function

5080121

Merge remote-tracking branch 'origin/dev' into wes-wgs-ui

10e8659

jklugherz requested a review from hanars October 24, 2024 14:16

hanars reviewed Oct 27, 2024

View reviewed changes

jklugherz added 2 commits October 28, 2024 11:49

warnings

9351898

every genotypes cn

b434d40

hanars approved these changes Oct 28, 2024

View reviewed changes

jklugherz merged commit 91ad846 into dev Oct 28, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update genotype display for multiple sample types #4437

update genotype display for multiple sample types #4437

jklugherz commented Oct 16, 2024 •

edited

Loading

hanars Oct 16, 2024

hanars Oct 21, 2024

hanars Oct 21, 2024

hanars Oct 21, 2024

hanars Oct 22, 2024

jklugherz Oct 23, 2024

hanars Oct 21, 2024

hanars Oct 22, 2024

hanars Oct 22, 2024

hanars Oct 22, 2024

jklugherz Oct 23, 2024

jklugherz Oct 24, 2024

hanars Oct 27, 2024

hanars Oct 22, 2024

jklugherz Oct 23, 2024

hanars Oct 22, 2024

hanars Oct 22, 2024

hanars Oct 22, 2024

jklugherz Oct 23, 2024

hanars Oct 27, 2024

jklugherz Oct 28, 2024

hanars Oct 28, 2024

jklugherz Oct 28, 2024

hanars Oct 27, 2024

jklugherz Oct 28, 2024

update genotype display for multiple sample types #4437

update genotype display for multiple sample types #4437

Conversation

jklugherz commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklugherz commented Oct 16, 2024 •

edited

Loading