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-1768] Add InFormatter for unpaired FASTQ. #1769

Merged
merged 1 commit into from Oct 31, 2017

Conversation

Projects
3 participants
@fnothaft
Member

fnothaft commented Oct 19, 2017

Resolves #1768. Still testing; not ready for merge. Thoughts on how to handle the configuration properties that are shared with InterleavedFASTQInFormatter? I'd rather not duplicate them.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Oct 19, 2017

Member

Thoughts on how to handle the configuration properties that are shared with InterleavedFASTQInFormatter? I'd rather not duplicate them.

The way you have them now, keyed by fields on FragmentRDD, seems fine to me. Or are you asking a different question?

Member

heuermh commented Oct 19, 2017

Thoughts on how to handle the configuration properties that are shared with InterleavedFASTQInFormatter? I'd rather not duplicate them.

The way you have them now, keyed by fields on FragmentRDD, seems fine to me. Or are you asking a different question?

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 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/2436/
Test PASSed.

AmplabJenkins commented Oct 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/2436/
Test PASSed.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 19, 2017

Member

The way you have them now, keyed by fields on FragmentRDD, seems fine to me. Or are you asking a different question?

No, that's exactly what I'm asking. I guess what I don't like about having them in FragmentRDD is that now you're using them with AlignmentRecordRDD in addition to FragmentRDD. Just seems a bit counterintuitive/awkward for a user.

Member

fnothaft commented Oct 19, 2017

The way you have them now, keyed by fields on FragmentRDD, seems fine to me. Or are you asking a different question?

No, that's exactly what I'm asking. I guess what I don't like about having them in FragmentRDD is that now you're using them with AlignmentRecordRDD in addition to FragmentRDD. Just seems a bit counterintuitive/awkward for a user.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 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/2437/
Test PASSed.

AmplabJenkins commented Oct 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/2437/
Test PASSed.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Oct 23, 2017

Member

No, that's exactly what I'm asking. I guess what I don't like about having them in FragmentRDD is that now you're using them with AlignmentRecordRDD in addition to FragmentRDD. Just seems a bit counterintuitive/awkward for a user.

I see. How about adding those key fields to both AlignmentRecordRDD and FragmentRDD?

Alternatively, we could put them onGenotypeRDD and then refer by specific subclass (AlignmentRecordRDD.WRITE_SUFFIXES) but that is somewhat poor code style.

Member

heuermh commented Oct 23, 2017

No, that's exactly what I'm asking. I guess what I don't like about having them in FragmentRDD is that now you're using them with AlignmentRecordRDD in addition to FragmentRDD. Just seems a bit counterintuitive/awkward for a user.

I see. How about adding those key fields to both AlignmentRecordRDD and FragmentRDD?

Alternatively, we could put them onGenotypeRDD and then refer by specific subclass (AlignmentRecordRDD.WRITE_SUFFIXES) but that is somewhat poor code style.

@heuermh heuermh added this to the 0.23.0 milestone Oct 24, 2017

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Oct 24, 2017

Member

Created fnothaft#21 to address comment above

Member

heuermh commented Oct 24, 2017

Created fnothaft#21 to address comment above

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 25, 2017

Member

Alternatively, we could put them on GenotypeRDD and then refer by specific subclass (AlignmentRecordRDD.WRITE_SUFFIXES) but that is somewhat poor code style.

I assume that by GenotypeRDD, you meant RecordGroupGenomicRDD?

Member

fnothaft commented Oct 25, 2017

Alternatively, we could put them on GenotypeRDD and then refer by specific subclass (AlignmentRecordRDD.WRITE_SUFFIXES) but that is somewhat poor code style.

I assume that by GenotypeRDD, you meant RecordGroupGenomicRDD?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 25, 2017

Member

I'm fine with fnothaft#21 for now, so let's go with that.

Member

fnothaft commented Oct 25, 2017

I'm fine with fnothaft#21 for now, so let's go with that.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 25, 2017

Member

@heuermh Do you want me to squash down your commit into mine, or leave it separate?

Member

fnothaft commented Oct 25, 2017

@heuermh Do you want me to squash down your commit into mine, or leave it separate?

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 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/2454/
Test PASSed.

AmplabJenkins commented Oct 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/2454/
Test PASSed.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Oct 25, 2017

Member

Squash is ok

Member

heuermh commented Oct 25, 2017

Squash is ok

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 25, 2017

Member

Squash is ok

Sounds good! I am going to test this later today, and will squash this down once it is OK to merge.

Member

fnothaft commented Oct 25, 2017

Squash is ok

Sounds good! I am going to test this later today, and will squash this down once it is OK to merge.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 30, 2017

Member

I've tested this and it is good to go. Squashing now.

Member

fnothaft commented Oct 30, 2017

I've tested this and it is good to go. Squashing now.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 30, 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/2456/
Test PASSed.

AmplabJenkins commented Oct 30, 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/2456/
Test PASSed.

@heuermh heuermh merged commit 22470c4 into bigdatagenomics:master Oct 31, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
default Merged build finished.
Details
@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Oct 31, 2017

Member

Thank you, @fnothaft

Member

heuermh commented Oct 31, 2017

Thank you, @fnothaft

@heuermh heuermh added this to Completed in Release 0.23.0 Jan 4, 2018

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