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

Refactor two VariantEval methods to allow subclasses to override #5998

Merged
merged 1 commit into from
Jun 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ private void checkForIncompatibleEvaluatorsAndStratifiers( final List<VariantStr
for ( Class<? extends VariantEvaluator> ec : evaluationClasses )
if ( vs.getIncompatibleEvaluators().contains(ec) )
throw new CommandLineException.BadArgumentValue("ST and ET",
"The selected stratification " + vs.getName() +
"The selected stratification " + vs.getName() +
" and evaluator " + ec.getSimpleName() +
" are incompatible due to combinatorial memory requirements." +
" Please disable one");
Expand All @@ -427,11 +427,21 @@ final void createStratificationStates(final List<VariantStratifier> stratificati

logger.info("Creating " + stratManager.size() + " combinatorial stratification states");
for ( int i = 0; i < stratManager.size(); i++ ) {
EvaluationContext ec = new EvaluationContext(this, evaluationObjects);
EvaluationContext ec = createEvaluationContext(evaluationObjects);
stratManager.set(i, ec);
}
}

/**
* Create the EvaluationContext (new instance) for the provided set of VariantEvaluators.
*
* @param evaluationObjects The list of VariantEvaluator classes
* @return The EvaluationContext for this set of VariantEvaluator classes
*/
protected EvaluationContext createEvaluationContext(final Set<Class<? extends VariantEvaluator>> evaluationObjects) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its pro forma, but please add descriptions of the param and return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

return new EvaluationContext(this, evaluationObjects);
}

private class PositionAggregator {
private SimpleInterval i = null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ public abstract class VariantEvaluator implements Comparable<VariantEvaluator> {
private VariantEval walker;
private final String simpleName;

protected VariantEvaluator(String simpleName) {
this.simpleName = simpleName;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The other constructor (below) should delegate to this one now: this(getClass().getSimpleName()), and this should have javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, but getClass() cant be called before the supertype constructor is called? is there a way around that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be fine.


protected VariantEvaluator() {
this.simpleName = getClass().getSimpleName();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@
import org.broadinstitute.hellbender.tools.walkers.varianteval.stratifications.manager.StratificationManager;

import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;

public final class EvaluationContext {
public class EvaluationContext {
// NOTE: must be hashset to avoid O(log n) cost of iteration in the very frequently called apply function
final VariantEval walker;
private final ArrayList<VariantEvaluator> evaluationInstances;
private final List<VariantEvaluator> evaluationInstances;
private final Set<Class<? extends VariantEvaluator>> evaluationClasses;

public EvaluationContext(final VariantEval walker, final Set<Class<? extends VariantEvaluator>> evaluationClasses) {
Expand All @@ -26,7 +27,7 @@ public EvaluationContext(final VariantEval walker, final Set<Class<? extends Var
private EvaluationContext(final VariantEval walker, final Set<Class<? extends VariantEvaluator>> evaluationClasses, final boolean doInitialize) {
this.walker = walker;
this.evaluationClasses = evaluationClasses;
this.evaluationInstances = new ArrayList<VariantEvaluator>(evaluationClasses.size());
this.evaluationInstances = new ArrayList<>(evaluationClasses.size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks.


for ( final Class<? extends VariantEvaluator> c : evaluationClasses ) {
try {
Expand All @@ -41,13 +42,31 @@ private EvaluationContext(final VariantEval walker, final Set<Class<? extends Va
}
}

/**
* Return a list of instances of each VariantEvaluator (see getEvaluationClasses). Note: elements of this list can be null.
*
* @return The list of VariantEvaluator instances
*/
public List<VariantEvaluator> getEvaluationInstances() {
return evaluationInstances;
}

/**
* Returns a set of VariantEvaluator classes to be used
*
* @return The set of VariantEvaluator classes to be used
*/
public Set<Class<? extends VariantEvaluator>> getEvaluationClasses() {
return evaluationClasses;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fill out @return.

/**
* Returns a sorted set of VariantEvaluators
*
* @return
* @return A sorted set of VariantEvaluator instances
*/
public final TreeSet<VariantEvaluator> getVariantEvaluators() {
return new TreeSet<VariantEvaluator>(evaluationInstances);
return new TreeSet<>(evaluationInstances);
}

public final void apply(ReferenceContext referenceContext, ReadsContext readsContext, FeatureContext featureContext, VariantContext comp, VariantContext eval) {
Expand Down