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

Documentation cleanup and minor refactor on the consensus package. #1055

Merged
merged 1 commit into from Jul 6, 2016

Conversation

Projects
None yet
3 participants
@fnothaft
Member

fnothaft commented Jun 25, 2016

  • Moved org.bdgenomics.adam.models.Concensus to org.bdgenomics.adam.algorithms.consensus.Consensus.
  • Made all classes in org.bdgenomics.adam.algorithms.consensus package private to org.bdgenomics.adam, along with IndelTable and NormalizationUtils.
  • Fleshed out documentation for all classes and methods in org.bdgenomics.adam.algorithms.consensus.Consensus.
  • Refactored Consensus method to use ReferenceRegion instead of start/end to check overlap bounds. This led to the discovery of an off by one error in ConsensusSuite, which was then fixed.
@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jun 25, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1283/
Test PASSed.

AmplabJenkins commented Jun 25, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1283/
Test PASSed.

* @note This method can only generate a single consensus from a read. Reads
* with more than one INDEL operator are ignored.
*
* @param sequence Read sequence.

This comment has been minimized.

@heuermh

heuermh Jun 27, 2016

Member

@param comments don't need to be full sentences

@heuermh

heuermh Jun 27, 2016

Member

@param comments don't need to be full sentences

* with more than one INDEL operator are ignored.
*
* @param sequence Read sequence.
* @param start The start position of the read alignment.

This comment has been minimized.

@heuermh

heuermh Jun 27, 2016

Member

e.g. I would prefer @param start start position of the read alignment

@heuermh

heuermh Jun 27, 2016

Member

e.g. I would prefer @param start start position of the read alignment

* @param sequence Read sequence.
* @param start The start position of the read alignment.
* @param cigar The CIGAR string for the local alignment of the read.
* @return Returns an optional consensus sequence if there is exactly one

This comment has been minimized.

@heuermh

heuermh Jun 27, 2016

Member

@return an optional consensus sequence...

@heuermh

heuermh Jun 27, 2016

Member

@return an optional consensus sequence...

*
* @throws IllegalArgumentException If the consensus doesn't overlap with the
* provided reference sequence.
*

This comment has been minimized.

@heuermh

heuermh Jun 27, 2016

Member

@throws IllegalArgumentException if the consensus sequence...

@heuermh

heuermh Jun 27, 2016

Member

@throws IllegalArgumentException if the consensus sequence...

Show outdated Hide outdated ...g/bdgenomics/adam/algorithms/consensus/ConsensusGeneratorFromReads.scala
* the read. We dedup the consensuses to remove any INDELs observed in
* multiple reads and return.
*
* @pararm Reads to search for INDELs.

This comment has been minimized.

@heuermh

heuermh Jun 27, 2016

Member

@pararm typo

@heuermh

heuermh Jun 27, 2016

Member

@pararm typo

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jun 27, 2016

Member

Left some comments regarding doc comment style.

The style I suggest is more in line with other major Java code bases, such as Apache Commons and Google Guava. It does not match the style of comments already in the ADAM code base, so perhaps the general open source principle of matching the style already in the code should win.

Member

heuermh commented Jun 27, 2016

Left some comments regarding doc comment style.

The style I suggest is more in line with other major Java code bases, such as Apache Commons and Google Guava. It does not match the style of comments already in the ADAM code base, so perhaps the general open source principle of matching the style already in the code should win.

Show outdated Hide outdated ...g/bdgenomics/adam/algorithms/consensus/ConsensusGeneratorFromReads.scala
@@ -68,7 +76,16 @@ class ConsensusGeneratorFromReads extends ConsensusGenerator {
}
/**
* Generates concensus sequences from reads with indels.
* Generates concensus sequences from reads with INDELs.

This comment has been minimized.

@heuermh

heuermh Jun 27, 2016

Member

concensusconsensus

@heuermh

heuermh Jun 27, 2016

Member

concensusconsensus

Documentation cleanup and minor refactor on the consensus package.
* Moved `org.bdgenomics.adam.models.Concensus` to
  `org.bdgenomics.adam.algorithms.consensus.Consensus`.
* Made all classes in `org.bdgenomics.adam.algorithms.consensus` package private
  to `org.bdgenomics.adam`, along with `IndelTable` and `NormalizationUtils`.
* Fleshed out documentation for all classes and methods in
  `org.bdgenomics.adam.algorithms.consensus.Consensus`.
* Refactored `Consensus` method to use `ReferenceRegion` instead of
  `start`/`end` to check overlap bounds. This led to the discovery of an off by
  one error in `ConsensusSuite`, which was then fixed.
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jul 4, 2016

Member

Addressed typos and rebased.

Member

fnothaft commented Jul 4, 2016

Addressed typos and rebased.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jul 4, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1297/
Test PASSed.

AmplabJenkins commented Jul 4, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1297/
Test PASSed.

@heuermh heuermh merged commit 9a4a1b7 into bigdatagenomics:master Jul 6, 2016

1 check passed

default Merged build finished.
Details
@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jul 6, 2016

Member

Thank you, @fnothaft!

Member

heuermh commented Jul 6, 2016

Thank you, @fnothaft!

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