A few improvements #966

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
@ryan-williams
Member

ryan-williams commented Mar 15, 2016

A couple random things I had laying around:

  • most substantive change: allow a minPartitions parameter to the various interval-file-loading functions, which wrap sc.textFile.
  • Thread a record-group parameter through some places that it was missing.
  • Style nits.

Fixes #969 #968

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Mar 15, 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/1104/
Test PASSed.

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

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Mar 15, 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/1105/
Test PASSed.

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

- projection: Option[Schema] = None): RDD[Feature] = {
+ def loadFeatures(filePath: String,
+ projection: Option[Schema],
+ minPartitions: Int): RDD[Feature] = {

This comment has been minimized.

@heuermh

heuermh Mar 16, 2016

Member

Could you surface this option on the command line (e.g. Features2ADAM.scala)?

@heuermh

heuermh Mar 16, 2016

Member

Could you surface this option on the command line (e.g. Features2ADAM.scala)?

This comment has been minimized.

@ryan-williams

ryan-williams Mar 16, 2016

Member

good idea, done

@ryan-williams

ryan-williams Mar 16, 2016

Member

good idea, done

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Mar 16, 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/1106/
Test PASSed.

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

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Mar 16, 2016

Member

LGTM, thanks

If you wouldn't mind, how about three issues (min partitions, record groups, style fixes)?

Member

heuermh commented Mar 16, 2016

LGTM, thanks

If you wouldn't mind, how about three issues (min partitions, record groups, style fixes)?

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Mar 17, 2016

Member

thx @heuermh I updated with two issues, I left off the style nits bc it seems ok for those to piggyback on the others but lmk if you want a 3rd one

Member

ryan-williams commented Mar 17, 2016

thx @heuermh I updated with two issues, I left off the style nits bc it seems ok for those to piggyback on the others but lmk if you want a 3rd one

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
Member

ryan-williams commented Mar 24, 2016

bump

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Mar 24, 2016

Member

@fnothaft volunteered to merge this, grouping the commits by issue

Member

heuermh commented Mar 24, 2016

@fnothaft volunteered to merge this, grouping the commits by issue

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Mar 24, 2016

Member

cool, sorry for the trouble, lmk if you want me to break them out some
other way myself!

On Thu, Mar 24, 2016 at 1:37 PM Michael L Heuer notifications@github.com
wrote:

@fnothaft https://github.com/fnothaft volunteered to merge this,
grouping the commits by issue


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#966 (comment)

Member

ryan-williams commented Mar 24, 2016

cool, sorry for the trouble, lmk if you want me to break them out some
other way myself!

On Thu, Mar 24, 2016 at 1:37 PM Michael L Heuer notifications@github.com
wrote:

@fnothaft https://github.com/fnothaft volunteered to merge this,
grouping the commits by issue


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#966 (comment)

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 24, 2016

Member

Just pinged you on gitter @ryan-williams but if you can rebase I will merge. I was going to yesterday but got distracted.

Member

fnothaft commented Mar 24, 2016

Just pinged you on gitter @ryan-williams but if you can rebase I will merge. I was going to yesterday but got distracted.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Mar 24, 2016

Member

rebased; picked up two tiny style nits that weren't there before also jfyi d60b7fc

Member

ryan-williams commented Mar 24, 2016

rebased; picked up two tiny style nits that weren't there before also jfyi d60b7fc

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 24, 2016

Member

Thanks for the heads up! Will let the tests run and review.

Member

fnothaft commented Mar 24, 2016

Thanks for the heads up! Will let the tests run and review.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Mar 24, 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/1115/
Test PASSed.

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

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 24, 2016

Member

Thanks @ryan-williams! I squashed down to 0d1bfd2, af5f733, and 13e7346 and merged.

Member

fnothaft commented Mar 24, 2016

Thanks @ryan-williams! I squashed down to 0d1bfd2, af5f733, and 13e7346 and merged.

@fnothaft fnothaft closed this Mar 24, 2016

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