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

Funcotation Engine Refactoring #5134

Merged
merged 4 commits into from
Sep 19, 2018
Merged

Funcotation Engine Refactoring #5134

merged 4 commits into from
Sep 19, 2018

Conversation

LeeTL1220
Copy link
Contributor

  • Added a FuncotationEngine class. This is meant to decouple the CLI from the funcotation generation.
  • Moved the querying of datasources into the funcotation factories. This means that the funcotation factory generates the funcotations using a feature context, instead of a map of feature inputs.
  • Funcotation factories now encapsulate the feature input of the source file. Non-locatable funcotation factories will have a null for this attribute.

@LeeTL1220 LeeTL1220 requested a review from jonn-smith August 24, 2018 19:04
@droazen droazen self-requested a review August 24, 2018 19:07
@droazen droazen self-assigned this Aug 24, 2018
@droazen droazen changed the title Funcotation Enginer Refactoring Funcotation Engine Refactoring Aug 24, 2018
@jonn-smith
Copy link
Collaborator

@LeeTL1220 can you fix broken build prior to review?

@LeeTL1220
Copy link
Contributor Author

@jonn-smith @droazen I forgot to run the tests on my laptop before PR. Anyway, should be okay now. If not, I'll fix (again).

@LeeTL1220
Copy link
Contributor Author

@jonn-smith @droazen Had to make two very small changes. Should be ready for review if tests pass.

@codecov-io
Copy link

codecov-io commented Aug 28, 2018 •
edited
Loading

Codecov Report

Merging #5134 into master will increase coverage by 65.599%.
The diff coverage is 90.181%.

@@               Coverage Diff                @@
##              master     #5134        +/-   ##
================================================
+ Coverage     21.259%   86.858%   +65.599%     
- Complexity      4663     30392     +25729     
================================================
  Files           1110      1833       +723     
  Lines          65822    139740     +73918     
  Branches       10536     15841      +5305     
================================================
+ Hits           13993    121375    +107382     
+ Misses         49755     12826     -36929     
- Partials        2074      5539      +3465
Impacted Files Coverage Δ Complexity Δ
...broadinstitute/hellbender/engine/FeatureInput.java 94.366% <ø> (+21.127%) 18 <0> (+6) ⬆️
...org/broadinstitute/hellbender/engine/GATKTool.java 91.589% <ø> (+21.356%) 100 <0> (+28) ⬆️
...uncotator/mafOutput/MafOutputRendererUnitTest.java 98.535% <ø> (ø) 16 <0> (?)
...decs/xsvLocatableTable/XsvLocatableTableCodec.java 82.716% <ø> (+74.074%) 60 <0> (+54) ⬆️
...te/hellbender/tools/funcotator/FuncotationMap.java 84.211% <ø> (+84.211%) 28 <0> (+28) ⬆️
...ataSources/xsv/SimpleKeyXsvFuncotationFactory.java 87.097% <ø> (+87.097%) 27 <0> (+27) ⬆️
...tools/funcotator/FuncotatorArgumentCollection.java 100% <100%> (ø) 1 <1> (?)
...tools/funcotator/DataSourceFuncotationFactory.java 86.364% <100%> (+69.697%) 16 <5> (+12) ⬆️
...oadinstitute/hellbender/engine/FeatureContext.java 78.571% <100%> (+67.761%) 28 <1> (+27) ⬆️
...cotator/dataSources/vcf/VcfFuncotationFactory.java 86.897% <100%> (+86.897%) 47 <0> (+47) ⬆️
... and 1699 more

jonn-smith
jonn-smith requested changes Aug 29, 2018
View reviewed changes
Copy link
Collaborator

@jonn-smith jonn-smith left a comment

Choose a reason for hiding this comment

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

Some minor comments and questions. In general it looks good.

I'm wondering if there is a way to restrict the access to some of the exposed methods that are used only in testing. My guess is no, but there could be some Java wizardry that can make it happen (aside from reflection, which would work but be really nasty).

Maybe @droazen or @lbergelson have an idea.

src/main/java/org/broadinstitute/hellbender/engine/FeatureManager.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/dataSources/DataSourceUtils.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/DataSourceFuncotationFactory.java Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/dataSources/DataSourceUtils.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
...st/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngineUnitTest.java Outdated Show resolved Hide resolved
droazen
droazen suggested changes Aug 29, 2018
View reviewed changes
Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

Added my comments @LeeTL1220. I've proposed a simple refactoring surrounding the creation of the FeatureInputs that should allow you to further reduce the amount of code left in Functotator itself.

src/main/java/org/broadinstitute/hellbender/engine/FeatureManager.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/Funcotator.java Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/DataSourceFuncotationFactory.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/dataSources/DataSourceUtils.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/engine/FuncotationEngine.java Outdated Show resolved Hide resolved
@droazen droazen assigned LeeTL1220 and unassigned droazen and jonn-smith Aug 30, 2018
@jonn-smith jonn-smith assigned jonn-smith and unassigned LeeTL1220 Sep 12, 2018
LeeTL1220 and others added 2 commits September 13, 2018 15:32
@LeeTL1220 @jonn-smith
Extraced a FuncotatorEngine from Funcotator.java
b822fc5 
A large refactoring of the architecture of Funcotator to make the
creation of annotations separate from the command line interface.

This will enable better testing of the annotation functionality by
allowing for unit tests that can test the created funcotations directly
(as opposed to requiring integration tests for the same purpose).
@jonn-smith
Fixing a small error made in the rebasing onto master.
5a3b039
@jonn-smith jonn-smith force-pushed the ll_funcotation_engine_refactor branch from cbed1ec to 5a3b039 Compare September 13, 2018 19:36
@jonn-smith jonn-smith assigned droazen and unassigned jonn-smith Sep 13, 2018
@jonn-smith
Copy link
Collaborator

@droazen back to you for a final look-over.

droazen
droazen suggested changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

Final review complete, back to @jonn-smith for some last changes. Merge after addressing this round of comments.

Summary: I think we can push a bit more code out of Funcotator and into the FuncotatorEngine, and I also would like to try to eliminate some parallel test-only codepaths.

src/main/java/org/broadinstitute/hellbender/engine/FeatureContext.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/FeatureManager.java Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/DataSourceFuncotationFactory.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/DataSourceFuncotationFactory.java Outdated Show resolved Hide resolved
...va/org/broadinstitute/hellbender/tools/funcotator/dataSources/vcf/VcfFuncotationFactory.java Outdated Show resolved Hide resolved
...oadinstitute/hellbender/tools/funcotator/dataSources/xsv/LocatableXsvFuncotationFactory.java Outdated Show resolved Hide resolved
...oadinstitute/hellbender/tools/funcotator/dataSources/xsv/LocatableXsvFuncotationFactory.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/FeatureContext.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java Outdated Show resolved Hide resolved
@droazen droazen assigned jonn-smith and unassigned droazen Sep 18, 2018
@jonn-smith jonn-smith merged commit 6391a47 into master Sep 19, 2018
@jonn-smith jonn-smith deleted the ll_funcotation_engine_refactor branch September 19, 2018 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonn-smith jonn-smith jonn-smith requested changes

@droazen droazen droazen requested changes

Assignees

@jonn-smith jonn-smith

Labels
Funcotator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LeeTL1220 @jonn-smith @codecov-io @droazen