Add region, allow empty regions, complete coordinate systems, GenomicPosition extends Position #34

ielis · 2021-01-12T03:13:34Z

Hi @julesjacobsen this is the today's work. I think that I completed all things we discussed today (including my notes below).

In addition to the tasks we agreed upon, I added BaseGenomicPosition, which is extended by PartialBreakend and DefaultGenomicPosition.

The tests where we call withStrand().withCoordinateSystem().withStrand().withCoordinateSystem() and we check that we got the same thing still remain to be added...

implement Comparable in Region subclasses
- naturalOrder as in GenomicRegion
behavior of coordinate system conversion of empty regions
- 1-based empty region is possible if we allow end < start
- let's allow this to not have to throw an exception
update genomic position implementations
- do we ever have an imprecise position that is not GenomicPosition as well? If not, then we can get rid of ImprecisePosition and create ImpreciseGenomicPosition
- not really, we use composition and we're fine
most of the methods like contains or overlapsWith() should be defined in Region. The extended versions that
also check for contig and strand should be defined in GenomicRegion
- let's make 2 versions, one for object the other is a pure function
remove endpoint from position, add CoordinateSystem to Region
contig should not be a region, use length to calculate the magic number for strand flipping
- function probably on a region
figure out the best place for getting the magic number for a contig that we need to flip strand of a region
make sure we can flip strand of an empty region in any coordinate system

…Position extends Position

julesjacobsen

I've a few minor points, but I'll address these.

julesjacobsen · 2021-01-12T10:34:32Z

src/main/java/org/monarchinitiative/variant/api/GenomicPosition.java

    }


    default int distanceTo(GenomicRegion other) {
-        if (contigId() != other.contigId()) {
+        if (contigId() != other.contigId())


Add missing braces

julesjacobsen · 2021-01-12T10:56:47Z

src/main/java/org/monarchinitiative/variant/api/GenomicPosition.java

@@ -174,7 +142,10 @@ default GenomicRegion toRegion(int upstream, int downstream) {
        } else if (upstream >= downstream) {
            throw new IllegalArgumentException("Cannot apply negative padding: " + upstream + ", " + downstream);
        }
-        return DefaultGenomicRegion.of(contig(), strand(), CoordinateSystem.ZERO_BASED, position().shift(upstream).asPrecise(), position().shift(downstream).asPrecise());
+


Why not just keep this as it was i.e. position.shift(upstream), position.shift(downstream) ?

I thought that keeping the confidence interval might be unnecessary overhead.

julesjacobsen · 2021-01-12T10:59:13Z

src/main/java/org/monarchinitiative/variant/api/GenomicPosition.java

@@ -131,7 +98,8 @@ default boolean isDownstreamOf(GenomicRegion other) {
     * @return the region
     */
    default GenomicRegion toRegion() {
-        return DefaultGenomicRegion.of(contig(), strand(), CoordinateSystem.ZERO_BASED, position().asPrecise(), position().shift(1).asPrecise());
+        return DefaultGenomicRegion.of(contig(), strand(), CoordinateSystem.ZERO_BASED,
+                Position.of(pos() - 1), Position.of(pos()));


why not use the shift method as before?

julesjacobsen · 2021-01-12T11:14:51Z

src/main/java/org/monarchinitiative/variant/api/Position.java

+    }
+
+    default int distanceTo(Region region) {
+        region = region.toOneBased();


don't re-assign this - use a new oneBased variable

julesjacobsen · 2021-01-12T11:23:44Z

src/main/java/org/monarchinitiative/variant/api/Region.java

+
+
+    default boolean overlapsWith(Region other) {
+        other = other.withCoordinateSystem(coordinateSystem());


Don't re-assign input

julesjacobsen · 2021-01-12T11:30:25Z

src/main/java/org/monarchinitiative/variant/api/impl/PartialBreakend.java

-                "contig=" + contig.id() +
-                ", id='" + id +
-                ", position=" + position +
-                ", strand=" + strand + '\'' +


Keep original - it's not helpful having the super.toString()

julesjacobsen · 2021-01-12T11:53:39Z

src/main/java/org/monarchinitiative/variant/api/impl/PartialBreakend.java

-    @Override
-    public Strand strand() {
-        return strand;
+    public static PartialBreakend of(Contig contig, String id, Strand strand, Position position) {


What is this doing now? Shouldn't this require a coordinate system?

I think Breakend (and in result PartialBreakend as well) should not be a GenomicPosition anymore, but rather a GenomicRegion. Breakend requires a coordinate system, which a position alone does not have.
If breakend is a GenomicRegion, then BreakendVariant delegates all it's GenomicRegion-ed bits to the left breakend.

This would remove an important use case for GenomicPosition. I would still like to keep the GenomicPosition, to be able to do things like creating splice donor site from an exon:

GenomicRegion exon = ...; GenomicRegion donor = exon.endGenomicPosition().withPadding(-3, 6);

But now I am starting to see that GenomicPosition needs to know whether it is open or closed. Otherwise, the code above would not generate the same results for exons in different coordinate system.

I would like to try to re-define the GenomicPosition as:

extends Position, Stranded<GenomicPosition>

has Contig and Endpoint

knows how to "open/close" itself

has natural order

has methods distanceTo(), isUpstreamOf(), etc.

knows how to convert itself into a region in a certain coordinate system

Do you think this sounds like a good thing?

Not sure I fully understand as I'm trying to refactor the code from this PR at the moment. Nothing major but I'm renaming CoordinateSystem.Endpoint to Boundary (probably ought to be BoundaryType) as the endpoint is really the start/end position on an interval.

Also noticed that the Boundary types were starting to leak outside of the CoordinateSystem e.g. in Region:

default int normalisedStart(Endpoint endpoint) { return start() + coordinateSystem().startDelta(endpoint); }

what does 'normalised' really mean here? it's actually the adjusted start for the provided coordinate system given the current one:

default int startWithCoordinateSystem(CoordinateSystem target) { return start() + coordinateSystem().startDelta(target); }

'but why are you hiding the Endpoint/Boundary?' you ask. Because of this:

normalisedStartPosition(requiredCoordinateSystem.startEndpoint()), normalisedEndPosition(requiredCoordinateSystem.endEndpoint()));

requiredCoordinateSystem.startEndpoint() was always paired with normalisedStartPosition and requiredCoordinateSystem.endEndpoint()) with normalisedEndPosition. The CoordinateSystem knows its boundary types, so delegate it to the class and don't pollute the rest of the codebase with unnecessary information and potential misuse:

return newRegionInstance(contig, strand, requiredCoordinateSystem, startPositionWithCoordinateSystem(requiredCoordinateSystem), endPositionWithCoordinateSystem(requiredCoordinateSystem));

It's all nice and clean like this, but why do I mention this now? Because you want to add Boundary (old Endpoint) to GenomicPosition.

This is a long-winded way of saying lets talk about this tomorrow! I'll hold off comitting/pushing my changes for the moment until we've worked through things together.

Add region, allow empty regions, complete coordinate systems, Genomic…

ca580e2

…Position extends Position

julesjacobsen approved these changes Jan 12, 2021

View reviewed changes

julesjacobsen merged commit cd58fd1 into coordinate-systemed-region Jan 12, 2021

ielis deleted the allow_empty_regions branch January 14, 2021 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add region, allow empty regions, complete coordinate systems, GenomicPosition extends Position #34

Add region, allow empty regions, complete coordinate systems, GenomicPosition extends Position #34

ielis commented Jan 12, 2021

julesjacobsen left a comment

julesjacobsen Jan 12, 2021

julesjacobsen Jan 12, 2021

ielis Jan 12, 2021

julesjacobsen Jan 12, 2021

julesjacobsen Jan 12, 2021

julesjacobsen Jan 12, 2021

julesjacobsen Jan 12, 2021

julesjacobsen Jan 12, 2021

ielis Jan 12, 2021

julesjacobsen Jan 12, 2021 •

edited

Loading



		default boolean overlapsWith(Region other) {
		other = other.withCoordinateSystem(coordinateSystem());

Add region, allow empty regions, complete coordinate systems, GenomicPosition extends Position #34

Add region, allow empty regions, complete coordinate systems, GenomicPosition extends Position #34

Conversation

ielis commented Jan 12, 2021

julesjacobsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julesjacobsen Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

julesjacobsen Jan 12, 2021 •

edited

Loading