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

FeatureRDD.apply() does not allow addition of other parameters with defaults in the case class #1439

Closed
devin-petersohn opened this Issue Mar 15, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@devin-petersohn
Member

devin-petersohn commented Mar 15, 2017

I blame Scala for this:

Scala will not allow us to have multiple overloaded methods to have parameters with defaults. Currently, we have a default for storageLevel in FeatureRDD Line 117.

In #1324 we need a default for the partition map when the objects are created shown on Line 242 of the PR branch, so I'm a little stuck. I am getting this error:

adam/adam-core/src/main/scala/org/bdgenomics/adam/rdd/feature/FeatureRDD.scala:105: error: in object FeatureRDD, multiple overloaded alternatives of apply define default arguments

We only have a couple of options and I don't know that I like any of them. @fnothaft @heuermh I'd love some thoughts on this.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Mar 15, 2017

Member

Yeah, I see the problem.

With FeatureRDD there are two paths for instantiation, one where the sequence dictionary is known (IntervalList format currently, and possibly GFF3 later if/when we add header support), and where it is not known and needs to be calculated (BED, GTF/GFF2, NarrowPeak formats). It'd be nice to have defaults whereever necessary, but if we have to pick and choose that's not the end of the world.

Member

heuermh commented Mar 15, 2017

Yeah, I see the problem.

With FeatureRDD there are two paths for instantiation, one where the sequence dictionary is known (IntervalList format currently, and possibly GFF3 later if/when we add header support), and where it is not known and needs to be calculated (BED, GTF/GFF2, NarrowPeak formats). It'd be nice to have defaults whereever necessary, but if we have to pick and choose that's not the end of the world.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 15, 2017

Member

I'd actually rather those two paths be two separate methods without defaults.

Member

fnothaft commented Mar 15, 2017

I'd actually rather those two paths be two separate methods without defaults.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Mar 15, 2017

Member

It didn't really feel right passing storageLevel around, especially to a constructor. Perhaps the creation of the sequence dictionary should move to where the native file format parsing happens instead?

Member

heuermh commented Mar 15, 2017

It didn't really feel right passing storageLevel around, especially to a constructor. Perhaps the creation of the sequence dictionary should move to where the native file format parsing happens instead?

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