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.

Sign up for GitHub

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

Jump to bottom

ReferenceRegion shouldn't extend Ordered #511

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

ReferenceRegion shouldn't extend Ordered #511

tdanford opened this issue Nov 28, 2014 · 1 comment
Assignees
@tdanford
Labels
discussion enhancement in progress!

Comments

@tdanford
Copy link
Contributor

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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
8c83c09 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
47052c8 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
d6613cf 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
e2a92a5 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
4a28c96 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
2d89a92 
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 tdanford mentioned this issue Dec 2, 2014
Closed
tdanford added a commit to tdanford/adam that referenced this issue Dec 18, 2014
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
2af48b4 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
44cc73d 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
2d71bd1 
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 fnothaft mentioned this issue Mar 1, 2015
Closed
tdanford added a commit to tdanford/adam that referenced this issue Mar 18, 2015
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
6621a4d 
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
@tdanford
[ADAM-511] Provide Ordering for ReferenceRegion
f41797d 
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
Copy link
Member

fnothaft commented Jul 6, 2016

Resolved since at least #624.

@fnothaft fnothaft closed this as completed Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tdanford tdanford

Labels
discussion enhancement in progress!
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tdanford @fnothaft