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

SNPCoverage: Exposed params for user customisation #951

Merged
merged 1 commit into from Feb 22, 2018

Conversation

Projects
None yet
3 participants
@nathanhaigh
Contributor

nathanhaigh commented Jan 4, 2018

Exposed 2 parameters to the user via the config file. They set the
minimum proportion (indicatorProp) and minimum depth
(indicatorDepth) of alternative alleles required to render the
SNP indicator below a SNPCoverage track.

Default values are chosen so that indicators are displayed (almost)
as they would have been before this commit. The only difference is
that now SNP indicators will also display if the alternative alleles
are exactly half the total coverage at a site. This is due to a change
in a conditional from ">" to ">=".

Expose some params for user customisation.
Exposed 2 parameters to the user via the config file. They set the
minimum proportion ('indicatorProp') and minimum depth
('indicatorDepth') of alternative alleles required to render the
SNP indicator below a SNPCoverage track.

Default values are chosen so that indicators are displayed (almost)
as they would have been before this commit. The only difference is
that now SNP indicators will also display if the alternative alleles
are exactly half the total coverage at a site. This is due to a change
in a conditional from ">" to ">=".
@nathanhaigh

This comment has been minimized.

Contributor

nathanhaigh commented Jan 4, 2018

This PR is useful as it helps as it allows the user to increase the proportion and depth of evidence required to render the indicators and will help avoid indicators being rendered in noisy/low coverage regions.

@nathanhaigh

This comment has been minimized.

Contributor

nathanhaigh commented Jan 24, 2018

Should be an easy merge? ping @nathandunn

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Jan 24, 2018

I think that @rbuels is in the zone right now if he has time to take a look and test, otherwise I'll take a look tomorrow. Overall, easy merge, and the changes look good.

@rbuels rbuels self-assigned this Jan 24, 2018

@rbuels rbuels modified the milestones: 1.12.0, 1.12.4 Jan 24, 2018

@rbuels rbuels self-requested a review Jan 24, 2018

@rbuels

The code looks good, we just need a little style tweak and then some verbiage on the wiki to document it.
@nathandunn do you have time to test the functionality?

@@ -143,14 +147,17 @@ return declare( [WiggleXY, AlignmentsMixin],
drawRectangle( 'reference', toY( score.total() ), originY-toY( score.get('reference'))+1, fRect);
});
var indicator_min_height_prop = this.config.indicatorProp;
var indicator_min_height = this.config.indicatorDepth;

This comment has been minimized.

@rbuels

rbuels Jan 24, 2018

Collaborator

Could you change the two variables above to be camelCase?

This comment has been minimized.

@nathandunn

nathandunn Jan 24, 2018

Contributor

@rbuels Sure, I can test the functionality

This comment has been minimized.

@nathandunn

nathandunn Jan 24, 2018

Contributor

Sure, I'll wait until @nathanhaigh pushes the changes so I'm testing the final.

This comment has been minimized.

@nathandunn

@rbuels rbuels requested a review from nathandunn Jan 24, 2018

@nathanhaigh

This comment has been minimized.

Contributor

nathanhaigh commented Jan 24, 2018

Great! I'll push the suggested changes next week...impending deadline and public holiday coming up.

@rbuels

This comment has been minimized.

Collaborator

rbuels commented Feb 2, 2018

@nathanhaigh

This comment has been minimized.

Contributor

nathanhaigh commented Feb 3, 2018

Hi @rbuels will get to this asap...tryimg to get a MS submitted at the moment.

@nathandunn nathandunn changed the base branch from master to dev Feb 7, 2018

@rbuels

This comment has been minimized.

Collaborator

rbuels commented Feb 12, 2018

@rbuels rbuels modified the milestones: 1.12.4, 1.12.5 Feb 14, 2018

@rbuels rbuels merged commit cb5e374 into GMOD:dev Feb 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathanhaigh nathanhaigh deleted the CroBiAd:SNPCoverage_expose_params branch Jul 13, 2018

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