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.

Sign up for GitHub

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

Jump to bottom

Exon curation #431

Open
wants to merge 5 commits into
base: feat/string-mut-name
Choose a base branch
from
Open

Exon curation #431

wants to merge 5 commits into from

Conversation

calvinlu3
Copy link
Contributor

@calvinlu3 calvinlu3 commented Aug 29, 2024 •
edited
Loading

Fixes https://github.com/oncokb/oncokb-pipeline/issues/411

Changes

  1. Redesign AddMutationModal
    • Consolidate alteration free text, info icon, and add button into a group
    • Show mutation list as badges instead of tabs
  2. Support exon curation
    • Allow exon alteration to be entered as free text or be selected from a dropdown
  3. Code refactoring: Managed add mutation modal state in a store and broke down modal into smaller components.

Product Review 1 Requests:

  • Include a name preview for mutation list
  • Fix issue where changing Type dropdown in mutation details doesn't update
  • Allow Exon range to be entered into exon dropdown (ie. Exon 2-4 Deletion)
  • Modify exon dropdown message with info icon to explain what "Any Exon 3-5 Deletion" means.
  • See if we can change the color contrast function so that green badge will show white text
  • Make string name select regular height
  • Update react-select grey border to align with the bootstrap input boxes
  • Update excluding badges to the same style as mutation list (try grey color, so doesn't conflict with mutation list)

Convert to tickets

  • Optional: For structural/CNA add another dropdown for consequence (Or combine into the type dropdown, where option is for example "Structural Varinat - Deletion")
  • Optional: Add mutation button and have the form show up (This is helpful when the curators don't know how to write a mutation in the proper format)

@calvinlu3 calvinlu3 changed the base branch from feat/string-mut-name to feat/exon-curation September 3, 2024 15:44
@calvinlu3 calvinlu3 force-pushed the exon-curation branch 3 times, most recently from 01522ec to c40dfb1 Compare September 3, 2024 21:09
@calvinlu3 calvinlu3 changed the base branch from feat/exon-curation to feat/string-mut-name September 3, 2024 21:14
@calvinlu3 calvinlu3 marked this pull request as ready for review September 3, 2024 21:14
zhx828
zhx828 requested changes Sep 5, 2024
View reviewed changes
Copy link
Member

@zhx828 zhx828 left a comment

Choose a reason for hiding this comment

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

I will come back to review again once the string name pr is merged.

src/main/java/org/mskcc/oncokb/curation/util/AlterationUtils.java
consequence.setTerm(SVConsequence.SV_DUPLICATION.name());
break;
case "deletion":
consequence.setTerm(SVConsequence.SV_DELETION.name());
Copy link
Member

Choose a reason for hiding this comment

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

Let's add break here just in case future changes forget to amend.

src/main/webapp/app/config/constants/regex.spec.ts
['Exon 14 Duplication', true],
['Exon 4 Insertion', true],
['Exon 4-8 Deletion', true],
['Exon 4 InSERTion', true],
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 normalize it to Exon 4 Insertion when user inputs InSERTion?

src/main/java/org/mskcc/oncokb/curation/service/MainService.java Outdated
Comment on lines 292 to 325
List<ProteinExonDTO> overlap = new ArrayList<>();
List<String> problematicExonAlts = new ArrayList<>();
for (String exonAlterationString : Arrays.asList(alteration.getAlteration().split("\\s*\\+\\s*"))) {
Integer exonNumber = Integer.parseInt(exonAlterationString.replaceAll("\\D*", ""));
if (exonNumber > 0 && exonNumber < proteinExons.size() + 1) {
overlap.add(proteinExons.get(exonNumber - 1));
} else {
problematicExonAlts.add(exonAlterationString);
}
}
if (problematicExonAlts.isEmpty()) {
overlap.sort(Comparator.comparingInt(ProteinExonDTO::getExon));
Boolean isConsecutiveExonRange =
overlap
.stream()
.map(ProteinExonDTO::getExon)
.reduce((prev, curr) -> (curr - prev == 1) ? curr : Integer.MIN_VALUE)
.orElse(Integer.MIN_VALUE) !=
Integer.MIN_VALUE;
if (isConsecutiveExonRange && overlap.size() > 0) {
alteration.setStart(overlap.get(0).getRange().getStart());
alteration.setEnd(overlap.get(overlap.size() - 1).getRange().getEnd());
}

annotationDTO.setExons(overlap);
} else {
StringBuilder sb = new StringBuilder();
sb.append("The following exon(s) do not exist: ");
sb.append(problematicExonAlts.stream().collect(Collectors.joining(", ")));
alterationWithStatus.setMessage(sb.toString());
alterationWithStatus.setType(EntityStatusType.ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

Need some comments and examples of what we trying to do here

Copy link
Contributor Author

@calvinlu3 calvinlu3 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code here is trying to validate the exons that the user entered. For instance in BRAF, if a user enters Exon 200 Deletion, we are adding some error status for AddMutationModal to pick up and display to user.
image

@calvinlu3 calvinlu3 added the feature Feature tag for release label Oct 1, 2024
@calvinlu3
Copy link
Contributor Author

@zhx828 This branch can be product reviewed.

  • Mutation list badge styling is still work in progress, but current incorporates the feedback of using "button outline" style.
  • For the "Excluding" alterations section - when alteration is clicked, then mutation details is shown. Might need some styling to indicate which excluded alteration we are currently viewing.

@calvinlu3 calvinlu3 requested a review from zhx828 October 1, 2024 13:06
@calvinlu3
Copy link
Contributor Author

@zhx828 Changes requested above have been addressed. If product looks good, then its also ready for a code review.

zhx828
zhx828 requested changes Oct 11, 2024
View reviewed changes
Copy link
Member

@zhx828 zhx828 left a comment

Choose a reason for hiding this comment

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

  • I think we still need to show how to curate Any 3-4 exons. I remember we used to have it?
  • This alteration box height is weird Screenshot 2024-10-11 at 3 49 34 PM
  • for the comment tooltip, can we add a delay before disappearing? This could avoid accidental leaving the tooltip and back. This happens more often when the tooltip is small and also has action in it
  • Incorrect alteration status. To reproduce: 1. add alteration with wrong reference 2. it shows warning color 3. save alteration. 4 modify alteration again. At this point, I want to see the alteration highlighted in yellow, but it's green

src/main/webapp/app/shared/modal/MutationModal/MutationListSection.tsx Outdated
</div>
)}
</div>
<div className="px-2 py-2">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the px-2

@calvinlu3
Copy link
Contributor Author

calvinlu3 commented Oct 15, 2024 •
edited
Loading

@zhx828 Updated based on feedback. Could you give some feedback on the following:
If you edit an exisiting mutation. Then click on the + Exon Button, the mutation list view turns into the exon dropdown view. There is currently no obvious way to go back to the mutation list view, so I added this "x" to close the dropdown view.
image
Maybe a left arrow that indicates going back to previous view?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhx828 zhx828 zhx828 requested changes

Assignees
No one assigned
Labels
feature Feature tag for release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@calvinlu3 @zhx828