Added in code to populate the name of VariantContext objects. #4571

jonn-smith · 2018-03-23T19:26:05Z

droazen · 2018-03-23T20:54:27Z

Before proceeding with this PR, let's check what GATK3 puts into the VariantContext source field, to see whether the behavior there is consistent with this branch.

droazen

Back to @jonn-smith with comments.

src/main/java/org/broadinstitute/hellbender/engine/FeatureDataSource.java

src/main/java/org/broadinstitute/hellbender/utils/test/VariantContextTestUtils.java

src/test/java/org/broadinstitute/hellbender/engine/FeatureDataSourceUnitTest.java

lbergelson · 2019-01-11T22:33:00Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/VariantContextTestUtils.java

 import java.io.File;
 import java.io.PrintStream;
 import java.util.*;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;

+// This should be:
+//import org.apache.logging.log4j.LogManager;


What's the deal with this change? can we remove these comments?

Because Intellij likes to auto resolve / update the import statements, I've found that the loggers occasionally get messed up and are switched to be the other includes which don't work.

I added this comment here so that I could preserve which loggers are correct. I'll reword the comment and make it clear. Initially this was above the logger lines, but since Intellij auto-organizes the imports they're no longer above the logging imports.

lbergelson · 2019-01-11T22:33:28Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/VariantContextTestUtils.java

@@ -2,15 +2,13 @@

 import com.google.common.annotations.VisibleForTesting;


Did you mean to include this file?

I think that's where the @VisibleForTesting attribute is defined, isn't it?

lbergelson

@jonn-smith This looks good 👍

codecov-io · 2019-01-14T22:23:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4416fd5). Click here to learn what that means.
The diff coverage is 11.765%.

@@            Coverage Diff             @@
##             master     #4571   +/-   ##
==========================================
  Coverage          ?   17.915%           
  Complexity        ?      8536           
==========================================
  Files             ?      1943           
  Lines             ?    146209           
  Branches          ?     16146           
==========================================
  Hits              ?     26194           
  Misses            ?    117360           
  Partials          ?      2655

Impacted Files	Coverage Δ	Complexity Δ
...e/hellbender/engine/FeatureDataSourceUnitTest.java	`1.488% <0%> (ø)`	`2 <0> (?)`
...institute/hellbender/engine/FeatureDataSource.java	`57.246% <66.667%> (ø)`	`32 <0> (?)`

droazen · 2019-01-16T22:03:28Z

@jonn-smith What's the status of this one?

jonn-smith · 2019-01-17T14:39:44Z

@droazen I rebased it and have been updating the comparison methods for VariantContext objects. I was talking with @lbergelson yesterday about postponing some of those changes (moving them into another branch/PR) and reverting to a version that will work to get this one in.

I haven't yet done that though.

droazen · 2019-01-17T16:22:38Z

@jonn-smith Can you explain briefly why the tests are failing? It seems like this is the sort of change that our test code should just ignore.

jonn-smith · 2019-02-07T20:13:50Z

Yes. We have two methods to compare whether variant context objects are equal.

I wanted to make it so that there was only 1 method that does this, so I tried to change the calls to only use one of them. However they do not actually check the same things.

I am going to move the changes into another branch / PR, but I haven't yet. After doing so I'll revert so that both comparisons remain and make sure the tests pass again.

jonn-smith · 2019-02-08T20:13:37Z

Reverted the variant context equality changes and put them in branch jts_variant_context_equality_methods.

gatk-bot · 2019-12-18T20:24:32Z

Travis reported job failures from build 28408
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	28408.13	logs
integration	oraclejdk8	28408.11	logs
integration	openjdk11	28408.12	logs
unit	openjdk8	28408.3	logs
python	openjdk8	28408.5	logs
integration	openjdk8	28408.2	logs

bbimber · 2021-03-03T14:39:31Z

@droazen @cmnbroad This PR is blocking #6973, and it's been stuck for a while. I would like to offer my help in doing any work to get this closed out. #7021 is related and I could offer help on that and updating needed tests if this could help get all this closed out.

bbimber · 2021-04-06T15:52:19Z

Hello - I'm writing to check on these PRs again. I appreciate these are not GATK's priority, but they're blocking #6973 and we've been stuck for a long time here. If there is additional work, testing or anything needed on this I am happy to offer my time if there's anything that would help get this done.

droazen · 2021-04-06T18:06:14Z

@bbimber Thanks for your patience, and apologies for the very long wait on this PR! We'll do our very best to get through these reviews as soon as our other priorities allow.

bbimber · 2021-04-19T18:43:18Z

@cmnbroad @droazen Good morning - I'm writing to follow up again. I appreciate that this not GATK's main priority, but I'm at the point where I need to either unblock this or fork GATK and publish another artifact so we can move our projects forward. The crux of what I'm trying to achieve with these changes is for our VariantQC tool; however, I'm bumping into several situations where MultiVariantWalkers need to be able to determine the FeatureInput source of the variants. I recognize that this and my original PR #6973 has taken a non-trivial amount of @cmnbroad time, but we have basically been stalled with an otherwise approved PR since Feb 8.

The PRs blocking that PR are this one and the related #7021. They both involve creating a way to connect VariantContext to FeatureInput - a capability that would benefit the GATK engine and I have been told by @cmnbroad you're interested in having. It seems like the primary problem associated with these changes is ensuring tests and VariantContext comparison code still works, since VariantContexts are going to tend to report a source. I dont know your internal conversations, so I'm guessing based on what's written in github. As I've said, I'd like to do whatever I can to get these changes into a form that takes as little of your effort as possible.

While this particular PR seems close, there is clearly some cleanup needed from what's there now, including code review from @lbergelson that no one ever fixed. Would it help if I put together #4571 and #7021 into a new branch where I also work through associated test changes and try to get this into one concise piece of code to review? Basically try to put everything together to be the minimal amount of work needed on your side? I havent heard anything one way or the other from GATK staff as to whether this would actually be helpful or not. Are there higher order design decisions that need to be considered that I'm not seeing?

I completely understand your time constraints - I'm just trying to figure out some solution that let's this go forward. Again, if we cant unblock this soon I'm going to probably fork GATK and need to start working off that project. Thanks.

jonn-smith added the Engine label Mar 23, 2018

jonn-smith assigned droazen Mar 23, 2018

jonn-smith requested a review from droazen March 23, 2018 19:26

droazen suggested changes Sep 10, 2018

View reviewed changes

droazen assigned jonn-smith and unassigned droazen Sep 10, 2018

cmnbroad mentioned this pull request Nov 21, 2018

Engine changes for VariantEval #5439

Open

jonn-smith force-pushed the jts_variant_context_names_4570 branch from b126583 to 4a3f9c9 Compare January 11, 2019 20:26

jonn-smith assigned lbergelson and unassigned jonn-smith Jan 11, 2019

jonn-smith requested a review from lbergelson January 11, 2019 22:17

lbergelson reviewed Jan 11, 2019

View reviewed changes

lbergelson approved these changes Jan 11, 2019

View reviewed changes

jonn-smith force-pushed the jts_variant_context_names_4570 branch from be65da5 to ad7bcbd Compare February 8, 2019 20:02

droazen assigned KevinCLydon and unassigned lbergelson Dec 16, 2019

jonn-smith added 5 commits December 18, 2019 14:40

Added in code to populate the name of VariantContext objects.

2d34510

Fixed an error in the unit tests by chaning equivalency asserts.

7bdd9f5

Addressing review comments.

3941b1b

Rebase updates.

eff742d

Addressed more review comments.

2b1429c

jonn-smith force-pushed the jts_variant_context_names_4570 branch from ca1a7c0 to 2b1429c Compare December 18, 2019 19:46

cmnbroad mentioned this pull request Nov 30, 2020

Switch VariantEval to MultiVariantWalkerGroupedOnStart #6973

Merged

bbimber mentioned this pull request Mar 3, 2021

Added new VariantContext comparison functionality for testing #7021

Open

bbimber mentioned this pull request Apr 21, 2021

Another approach to conditionally store FeatureInput name in VC #7219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added in code to populate the name of VariantContext objects. #4571

Added in code to populate the name of VariantContext objects. #4571

jonn-smith commented Mar 23, 2018

droazen commented Mar 23, 2018

droazen left a comment

lbergelson Jan 11, 2019

jonn-smith Feb 8, 2019

lbergelson Jan 11, 2019

jonn-smith Feb 8, 2019

lbergelson left a comment

codecov-io commented Jan 14, 2019 •

edited by codecov bot

Loading

droazen commented Jan 16, 2019

jonn-smith commented Jan 17, 2019

droazen commented Jan 17, 2019

jonn-smith commented Feb 7, 2019

jonn-smith commented Feb 8, 2019

gatk-bot commented Dec 18, 2019 •

edited

Loading

bbimber commented Mar 3, 2021

bbimber commented Apr 6, 2021

droazen commented Apr 6, 2021

bbimber commented Apr 19, 2021

		@@ -2,15 +2,13 @@

		import com.google.common.annotations.VisibleForTesting;

Added in code to populate the name of VariantContext objects. #4571

Are you sure you want to change the base?

Added in code to populate the name of VariantContext objects. #4571

Conversation

jonn-smith commented Mar 23, 2018

droazen commented Mar 23, 2018

droazen left a comment

Choose a reason for hiding this comment

lbergelson Jan 11, 2019

Choose a reason for hiding this comment

jonn-smith Feb 8, 2019

Choose a reason for hiding this comment

lbergelson Jan 11, 2019

Choose a reason for hiding this comment

jonn-smith Feb 8, 2019

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 14, 2019 • edited by codecov bot Loading

Codecov Report

droazen commented Jan 16, 2019

jonn-smith commented Jan 17, 2019

droazen commented Jan 17, 2019

jonn-smith commented Feb 7, 2019

jonn-smith commented Feb 8, 2019

gatk-bot commented Dec 18, 2019 • edited Loading

bbimber commented Mar 3, 2021

bbimber commented Apr 6, 2021

droazen commented Apr 6, 2021

bbimber commented Apr 19, 2021

codecov-io commented Jan 14, 2019 •

edited by codecov bot

Loading

gatk-bot commented Dec 18, 2019 •

edited

Loading