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

ReferenceRegion shouldn't extend Ordered #511

Closed
tdanford opened this Issue Nov 28, 2014 · 1 comment

Comments

Projects
None yet
2 participants
@tdanford
Contributor

tdanford commented Nov 28, 2014

Currently, ReferenceRegion extends Ordered (everything here also applies, mutatis mutandis, to ReferenceRegionWithOrientation).

This is a problem semantically, as according to Scala's docs (http://www.scala-lang.org/api/current/index.html#scala.math.Ordered) Ordered should only be used for types that have "a single, natural ordering" -- while we certainly can (and will) want to sort ReferenceRegions in multiple different ways.

Second, it has become a problem concretely as well: in defining some natural operations on RDDs of ReferenceRegions, I'm coming across a "diverging implicit expansion" error (http://stackoverflow.com/questions/4883906/what-is-a-diverging-implicit-expansion-error) that seems to be related to using Ordered on ReferenceRegions and subclasses of ReferenceRegion.

The solution I'm proposing to this is threefold:

  1. removed Ordered from the superclasses of ReferenceRegion,
  2. move the compareTo method in ReferenceRegion into the compare method of a ReferenceRegionOrdering, which will extend the Ordering trait (the correct trait we should be using when there is more than one ordering on a type), and
  3. provide an implicit Ordering in a new object, ReferenceRegionContext, that can be imported by classes that need the Ordering available.

@tdanford tdanford self-assigned this Nov 28, 2014

tdanford added a commit to tdanford/adam that referenced this issue Nov 28, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

tdanford added a commit to tdanford/adam that referenced this issue Nov 28, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

tdanford added a commit to tdanford/adam that referenced this issue Nov 28, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Dec 2, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Dec 2, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Dec 2, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Dec 18, 2014

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Feb 14, 2015

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Feb 26, 2015

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Mar 18, 2015

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.

tdanford added a commit to tdanford/adam that referenced this issue Mar 24, 2015

[ADAM-511] Provide Ordering for ReferenceRegion
As covered in issue bigdatagenomics#511, ReferenceRegion shouldn't extend Ordered, but
rather should be ordered by an Ordering value which can be imported
implicitly.  This will fix some "diverging implicit expansion" errors,
and bring us more in line with the Scala documentation on Ordered (which
is intended to be used in classes that have "a single, natural ordering"
-- a description that doesn't apply to ReferenceRegion).

This commit makes these changes, with the new Ordering provided
implicitly in ReferenceRegionContext.  It also includes corresponding
changes to the ReferenceRegionWithOrientation handling, and (finally)
includes changes to downstream classes that use(d) the Region.compareTo
method.

Also bundled in to this commit are two additional changes to
ReferenceRegion:
* Adding explanations to ReferenceRegion asserts.
* Adding 'extend' to ReferenceRegionWithOrientation, allowing a
  strand-specific extension of the region in a single direction.
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jul 6, 2016

Member

Resolved since at least #624.

Member

fnothaft commented Jul 6, 2016

Resolved since at least #624.

@fnothaft fnothaft closed this Jul 6, 2016

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