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

[ADAM-1377] Adding fragment InFormatter for Bowtie tab6 format #1491

Merged
merged 1 commit into from Apr 25, 2017

Conversation

@heuermh
Copy link
Member

@heuermh heuermh commented Apr 14, 2017

Fixes #1377. Work in progress, need to write tests and eliminate copy-pasta.

@coveralls
Copy link

@coveralls coveralls commented Apr 14, 2017

Coverage Status

Coverage decreased (-0.4%) to 81.344% when pulling 54adbc0 on heuermh:alt-tab56 into 04444aa on bigdatagenomics:master.

@coveralls
Copy link

@coveralls coveralls commented Apr 14, 2017

Coverage Status

Coverage decreased (-0.3%) to 81.459% when pulling 54adbc0 on heuermh:alt-tab56 into 04444aa on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 14, 2017

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

Copy link
Member

@fnothaft fnothaft left a comment

I think this is still WIP, but a few comments.

* the current or original read qualities should be written. True indicates
* to write the original qualities. The default is false.
*/
val WRITE_ORIGINAL_QUALITIES = "org.bdgenomics.adam.rdd.fragment.Tab6InFormatter.writeOriginalQualities"

This comment has been minimized.

@fnothaft

fnothaft Apr 15, 2017
Member

What say you to making these a constant in org.bdgenomics.adam.rdd.fragment.FragmentRDD or the like so that we don't repeat them across all the InFormatters?

This comment has been minimized.

@heuermh

heuermh Apr 15, 2017
Author Member

Yeah, that is what I meant by copy-pasta . . . I would've put them in an abstract class that both InFormatters extended from but was not so sure about the companion stuff.

* +<optional readname>
* ASCII quality scores
* }}}
* Prepare a single record for conversion to FASTQ and similar formats.

This comment has been minimized.

@fnothaft

fnothaft Apr 15, 2017
Member

I would expand what you mean by "prepare" here. Once I read through the method, I understood what you meant, but ideally I shouldn't need to read the source first.

}

// end line
os.write("\n".getBytes)

This comment has been minimized.

@fnothaft

fnothaft Apr 15, 2017
Member

I would compute the value of "\n".getBytes outside of the for loop.

@coveralls
Copy link

@coveralls coveralls commented Apr 17, 2017

Coverage Status

Coverage decreased (-0.3%) to 81.44% when pulling 65a5b61 on heuermh:alt-tab56 into 04444aa on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 17, 2017

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

@coveralls
Copy link

@coveralls coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.3%) to 82.003% when pulling 48b9ce0 on heuermh:alt-tab56 into 04444aa on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 19, 2017

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

@heuermh heuermh force-pushed the heuermh:alt-tab56 branch from 48b9ce0 to f687d1e Apr 24, 2017
@coveralls
Copy link

@coveralls coveralls commented Apr 24, 2017

Coverage Status

Coverage decreased (-0.02%) to 81.709% when pulling f687d1e on heuermh:alt-tab56 into 5b6a109 on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 24, 2017

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

Copy link
Member

@fnothaft fnothaft left a comment

LGTM! @heuermh is this ready for merge from your end?

@heuermh
Copy link
Member Author

@heuermh heuermh commented Apr 25, 2017

Yes, thank you.

@fnothaft
Copy link
Member

@fnothaft fnothaft commented Apr 25, 2017

Thanks! Any chance you could squash the 1st and 3rd commits down? If not, I will merge manually later.

@heuermh heuermh added this to the 0.23.0 milestone Apr 25, 2017
@heuermh heuermh force-pushed the heuermh:alt-tab56 branch from f687d1e to 5f9c874 Apr 25, 2017
@coveralls
Copy link

@coveralls coveralls commented Apr 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 81.709% when pulling 5f9c874 on heuermh:alt-tab56 into 5b6a109 on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 25, 2017

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

@fnothaft fnothaft merged commit dbe5c97 into bigdatagenomics:master Apr 25, 2017
2 of 3 checks passed
2 of 3 checks passed
coverage/coveralls Coverage decreased (-0.02%) to 81.709%
Details
codacy/pr Good work! A positive pull request.
Details
default Merged build finished.
Details
@fnothaft
Copy link
Member

@fnothaft fnothaft commented Apr 25, 2017

Merged! Thanks @heuermh!

@heuermh heuermh deleted the heuermh:alt-tab56 branch Apr 25, 2017
@heuermh heuermh added this to Completed in Release 0.23.0 May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.