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

Add discrete_long cna data type #4423

Closed
wants to merge 1 commit into from

Conversation

BasLee
Copy link

@BasLee BasLee commented Nov 16, 2022

Add DISCRETE_LONG as a possible CNA data type.

Fixes e2e test failures introduced by the new CNA long format of cBioPortal/cbioportal#9847

Changes

  • Add DISCRETE_LONG to data types
  • Add CnaDataTypes that contains both possible cna data types

Copy link
Member

@dippindots dippindots left a comment

Choose a reason for hiding this comment

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

This looks good to me, just make sure tests are passing before merging.

@@ -2694,7 +2695,7 @@ export class ResultsViewPageStore
profile =>
profile.molecularAlterationType ===
AlterationTypeConstants.COPY_NUMBER_ALTERATION &&
profile.datatype === DataTypeConstants.DISCRETE
CnaDataTypes.includes(profile.datatype)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? Or maybe just keep
profile.molecularAlterationType === AlterationTypeConstants.COPY_NUMBER_ALTERATION
is enough?

@@ -510,7 +514,7 @@ export function pickCopyNumberEnrichmentProfiles(profiles: MolecularProfile[]) {
(profile: MolecularProfile) =>
profile.molecularAlterationType ===
AlterationTypeConstants.COPY_NUMBER_ALTERATION &&
profile.datatype === 'DISCRETE'
CnaDataTypes.includes(profile.datatype)
Copy link
Member

Choose a reason for hiding this comment

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

same here

@BasLee
Copy link
Author

BasLee commented Nov 17, 2022

This looks good to me, just make sure tests are passing before merging.

Thanks for reviewing @dippindots!

On second thought, maybe this whole new DISCRETE_LONG datatype is not necessary? I tried to explain it here:
cBioPortal/cbioportal#9847 (comment)

Any thoughts?

@alisman
Copy link
Collaborator

alisman commented Nov 17, 2022

@BasLee @pvannierop as far as I can tell, there is no special treatment given to DISCRETE_LONG datatype in the frontend. If that's true, and will always be true, perhaps we should make an effort to hide it from the frontend? Apologies if I misunderstand.

@BasLee
Copy link
Author

BasLee commented Nov 17, 2022

Ok, we can shelve/delete this PR, in favor of the changes mentioned in cBioPortal/cbioportal#9847 (comment)

@alisman alisman closed this Nov 17, 2022
@alisman
Copy link
Collaborator

alisman commented Nov 17, 2022

We have decided to hide this new type (which is only relevant to import process)

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