Add script that allows for update to existing gene panel #7695

jtquach1 · 2020-07-16T03:09:03Z

Describe changes proposed in this pull request:

Update functionality
Add or remove genes from gene panel depending on list of genes in incoming vs. current panel

Any screenshots or GIFs?

Before:

After:

jtquach1 · 2020-07-16T22:02:49Z

These are some sketches I made regarding edge cases in comparing incoming with original genes.
updateGenePanel_cases.pdf

So far, I've tested intersection case 1 on pages 1 and 2, where A = imported (test panel to overwrite with) and B = original (test panel from database).

The first image corresponds to the Before and After in the PR's top comment. For the second image, I tested this using the TESTPANEL2 text file against TESTPANEL2 (unchanged).

jtquach1 · 2020-07-16T23:35:50Z

I've just tested cases 1-7, and the only changes I made were for case 1, where having an empty gene list was disallowed for importing. I changed that so case 1 allows the user to delete all genes from the original gene panel when importing an empty gene panel.
case_testing.pdf

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

Luke-Sikina · 2020-07-16T13:08:37Z

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

-        }
-    }
-}
+/*


I have a hunch that Windows is changing the newline characters in this file, hence the weird diff. I'll look around for solutions.

I think so too, as I get this message from doing commits:

jtquach@DESKTOP-IST3H2B:~/cbioportal_windows10$ git add core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java warning: LF will be replaced by CRLF in core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java. The file will have its original line endings in your working directory

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java

core/src/test/resources/seed_mini.sql

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/util/GenePanelUtil.java

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/util/GenePanelUtil.java

core/src/test/java/org/mskcc/cbio/portal/util/TestGenePanelUtil.java

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

core/src/test/java/org/mskcc/cbio/portal/util/TestGenePanelUtil.java

Luke-Sikina

This looks great. I left a few tiny changes, but for the most part, I think this is ready to ship.

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java

core/src/test/java/org/mskcc/cbio/portal/util/TestGenePanelUtil.java

n1zea144

Nice cleanup to existing code. Some comments for consideration.

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

n1zea144 · 2020-07-28T15:53:42Z

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java

+        GenePanel genePanel = DaoGenePanel.getGenePanelByStableId(stableId);
+
+        if (genePanel == null) {
+            ProgressMonitor.logWarning("Gene panel " + stableId + " does not exist in the database! Exiting.");


Just wondering if there was any thought to combining the update functionality into the Import script?

According to Luke, the import and update functionality should be separate from each other

@n1zea144 Leaving them as separate scripts makes it a bit harder to make the mistake of updating an existing gene panel when you instead meant to upload a new version of a gene panel for new samples. I think this is an easy enough mistake- the user copies the gene panel file, vims it, updates the genes, but forgets to change the name of the panel before running the script. By keeping the two scripts separate, we make it so a user that ignores warning messages and just presses enter a bunch cannot end up making this mistake.

n1zea144 · 2020-07-30T16:07:25Z

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

+                pstmt.setLong(2, canonicalGene.getEntrezGeneId());
+                pstmt.executeUpdate();
+            }
+            for (CanonicalGene canonicalGene : toRemove) {


It looks like its possible to remove all genes from the panel. Should we check this and if so, should we prevent this from happening ?

In UpdateGenePanel.java, the user prompt in importData() verifies the changes that will be made to the gene panel, like so:

Other than that, I asked Luke if importing an empty gene panel should allow for the removal of all existing genes, and he stated that behavior sounds reasonable. I suppose if the user wanted to add back the genes, they could pull up an older version of their input file with the gene names.
I'll ping Luke about whether after all genes are removed, that the gene panel should be removed from gene_panel_list as well

ah, so removal of all genes means stop the update script and don't make the deletions

It's not meaningful to have a panel in the database, which samples refer to in sample_profile, that contain no genes. I'm also not sure what the affect is on the web page. One approach is to remove the the panel and subsequent records in sample_profile (via cascade), but thats a bit messy and could happen by mistake.

Spoke with @Luke-Sikina via Slack. Best approach is not to prevent an update which results in empty panel in database.

Oh, does that mean that within UpdateGenePanel, checking if the incoming set of genes is empty and aborting the script is not needed?

I've just added the check on lines 77-80 on UpdateGenePanel, after the gene panel null check:

if (genePanel == null) { ProgressMonitor.logWarning("Gene panel " + stableId + " does not exist in the database! Exiting."); return; } if (canonicalGenes == null || canonicalGenes.isEmpty()) { ProgressMonitor.logWarning("Incoming gene panel is empty, which would result in the removal of all genes from gene panel " + stableId + ". Exiting."); return; }

(edited since I realized to use ProgressMonitor.logWarning instead of System.err.println)

Yeah, checking/aborting when the incoming gene list from the panel file is empty should suffice, thanks for the adjustment.

Thank you for the reviews!

I've just updated the markdown file again to reflect that trying to import an empty gene panel will abort the script.

n1zea144 · 2020-07-30T19:35:16Z

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java

+                pstmt.setLong(2, canonicalGene.getEntrezGeneId());
+                pstmt.executeUpdate();
+            }
+            for (CanonicalGene canonicalGene : toRemove) {


Yeah, checking/aborting when the incoming gene list from the panel file is empty should suffice, thanks for the adjustment.

Added UpdateGenePanel, based off ImportGenePanel Refactored and moved identical methods into new util file Updating allows user to add/remove genes from existing gene panel in database Allow empty gene panel to be inputted, but abort script if so Prompt user with Y/n to notify that this is an update and not import script Preview genes to be added/removed before finalizing Y/n prompt Unit tests for utility methods Updated ImportGenePanel calls Formatted code for readability Added javadocs Added null check in extractPropertyValue Added 12 testpanel genes and their aliases for testing Added Perl script and updated markdown documentation Edited markdown documentation on how to use the update script Co-authored-by: Luke Sikina <lucas.sikina@gmail.com>

jtquach1 requested a review from Luke-Sikina July 16, 2020 03:09

jtquach1 self-assigned this Jul 16, 2020

jtquach1 force-pushed the update_existing_gene_panel branch from 21b2133 to f0d9601 Compare July 16, 2020 13:43

jtquach1 changed the title ~~First draft of updating existing gene panel script~~ Add script that allows for update to existing gene panel Jul 17, 2020

jtquach1 commented Jul 17, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 17, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 19, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java Outdated Show resolved Hide resolved

Luke-Sikina reviewed Jul 20, 2020

View reviewed changes

jtquach1 commented Jul 20, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 20, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 20, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 20, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/util/GenePanelUtil.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 21, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/util/GenePanelUtil.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 21, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/util/GenePanelUtil.java Outdated Show resolved Hide resolved

jtquach1 commented Jul 21, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/util/GenePanelUtil.java Show resolved Hide resolved

jtquach1 force-pushed the update_existing_gene_panel branch from bc0843e to e9c9eec Compare July 21, 2020 20:08

Luke-Sikina requested changes Jul 22, 2020

View reviewed changes

Luke-Sikina reviewed Jul 22, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/scripts/UpdateGenePanel.java Show resolved Hide resolved

Luke-Sikina reviewed Jul 22, 2020

View reviewed changes

core/src/main/java/org/mskcc/cbio/portal/dao/DaoGenePanel.java Outdated Show resolved Hide resolved

jtquach1 force-pushed the update_existing_gene_panel branch from d591058 to dad1b5b Compare July 22, 2020 22:17

Luke-Sikina requested changes Jul 23, 2020

View reviewed changes

jtquach1 force-pushed the update_existing_gene_panel branch from 9f1c44b to d6c2b64 Compare July 23, 2020 19:15

Luke-Sikina approved these changes Jul 23, 2020

View reviewed changes

jtquach1 force-pushed the update_existing_gene_panel branch 5 times, most recently from c395620 to 075eb15 Compare July 24, 2020 18:41

Luke-Sikina requested a review from n1zea144 July 24, 2020 19:49

n1zea144 reviewed Jul 28, 2020

View reviewed changes

jtquach1 force-pushed the update_existing_gene_panel branch 3 times, most recently from 524a21e to d2b14f7 Compare July 28, 2020 17:49

jtquach1 requested a review from n1zea144 July 28, 2020 20:07

n1zea144 reviewed Jul 30, 2020

View reviewed changes

jtquach1 force-pushed the update_existing_gene_panel branch 2 times, most recently from 0468141 to 83c7b91 Compare July 30, 2020 16:57

jtquach1 requested a review from n1zea144 July 30, 2020 18:35

n1zea144 approved these changes Jul 30, 2020

View reviewed changes

jtquach1 force-pushed the update_existing_gene_panel branch from 83c7b91 to e483396 Compare July 30, 2020 20:14

Luke-Sikina merged commit ec73d82 into cBioPortal:master Aug 3, 2020

inodb added the feature label Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script that allows for update to existing gene panel #7695

Add script that allows for update to existing gene panel #7695

jtquach1 commented Jul 16, 2020 •

edited

jtquach1 commented Jul 16, 2020 •

edited

jtquach1 commented Jul 16, 2020 •

edited

Luke-Sikina Jul 16, 2020

jtquach1 Jul 20, 2020 •

edited

Luke-Sikina left a comment

n1zea144 left a comment

n1zea144 Jul 28, 2020

jtquach1 Jul 28, 2020

Luke-Sikina Jul 28, 2020

n1zea144 Jul 30, 2020 •

edited

jtquach1 Jul 30, 2020

jtquach1 Jul 30, 2020

n1zea144 Jul 30, 2020

n1zea144 Jul 30, 2020

jtquach1 Jul 30, 2020

jtquach1 Jul 30, 2020 •

edited

n1zea144 Jul 30, 2020

jtquach1 Jul 30, 2020

jtquach1 Jul 30, 2020

n1zea144 Jul 30, 2020

Add script that allows for update to existing gene panel #7695

Add script that allows for update to existing gene panel #7695

Conversation

jtquach1 commented Jul 16, 2020 • edited

Any screenshots or GIFs?

jtquach1 commented Jul 16, 2020 • edited

jtquach1 commented Jul 16, 2020 • edited

Choose a reason for hiding this comment

jtquach1 Jul 20, 2020 • edited

Choose a reason for hiding this comment

Luke-Sikina left a comment

Choose a reason for hiding this comment

n1zea144 left a 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

n1zea144 Jul 30, 2020 • edited

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

jtquach1 Jul 30, 2020 • edited

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

jtquach1 commented Jul 16, 2020 •

edited

jtquach1 commented Jul 16, 2020 •

edited

jtquach1 commented Jul 16, 2020 •

edited

jtquach1 Jul 20, 2020 •

edited

n1zea144 Jul 30, 2020 •

edited

jtquach1 Jul 30, 2020 •

edited