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

Making example code compatible with current ADAM build #1641

Closed

Conversation

@devin-petersohn
Copy link
Member

@devin-petersohn devin-petersohn commented Jul 26, 2017

Resolves #1633.

@coveralls
Copy link

@coveralls coveralls commented Jul 26, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling 01a649f on devin-petersohn:issue#1633 into c8a2202 on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Jul 26, 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/2286/
Test PASSed.

Copy link
Member

@fnothaft fnothaft left a comment

These changes are OK, but we're probably going to gut the intro as written, so I'd suggest we don't merge these changes.


For example, the following code snippet will print the top 10 21-mers in `NA2114` from 1000 Genomes.
For example, the following code snippet will print the top 10 21-mers in
`NA2114` from 1000 Genomes.

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

2114 -> 21144

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

Ping!

This comment has been minimized.

@devin-petersohn

devin-petersohn Aug 2, 2017
Author Member

Are we not just closing this PR? Or are we merging this with #1653? I only ask because your review comment suggested we don't merge.

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

Oh sorry, that's right. Yes, let's close this and revisit later.

// Generate, count and sort 21-mers
val kmers = reads.flatMap { read =>
read.getSequence.sliding(21).map(k => (k, 1L))
}.reduceByKey((k1: Long, k2: Long) => k1 + k2)
}.reduceByKey(_ + _)

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

Use countKmers(21) instead?

@devin-petersohn
Copy link
Member Author

@devin-petersohn devin-petersohn commented Jul 26, 2017

Why are we gutting the intro? I thought it was just the README that we were significantly overhauling.

@fnothaft
Copy link
Member

@fnothaft fnothaft commented Jul 26, 2017

Just like the README, the intro in the docs isn't really an intro, inasmuch as it is a jumble of links and miscellaneous examples.

@fnothaft fnothaft closed this Aug 2, 2017
@heuermh heuermh added this to the 0.23.0 milestone Dec 7, 2017
@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
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

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