Necessary Updates to the Mapper code [ENSCORESW-2168]. #332

avullo · 2018-11-13T16:50:17Z

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion;
Review the contributing guidelines for this repository; remember in particular:
- do not modify code without testing for regression
- provide simple unit tests to test the changes
- if you change the schema you must patch the test databases as well, see Updating the schema
- the PR must not fail unit testing

Description

This is a rebase onto master of branch experimental/mapper_update, which is meant to address most of the Mapper issues documented under [ENSCORESW-2168].

Use case

The current mapper code does not correctly handle several edge cases and emits an incorrect output. See [ENSCORESW-2168] for more details.

Benefits

A much more robust mapper code. Among several things, it's making the Genebuild Loutre import pipeline work correctly.

Possible Drawbacks

The Mapper::map_coordinates method could be slower under certain circumstances, i.e. when mapping from several non overlapping intervals as it has now to take into account the cost of building and querying an interval tree. Querying is more expensive than current binary search in case the tree is unbalanced. We might use the provided mutable tree implementation since we don't incur in the cost associated with keeping the tree balanced as we're not inserting or removing intervals after construction. This might also have the benefit of exploiting an existing XS implementation.

Testing

Have you added/modified unit tests to test the changes?

Yes

If so, do the tests pass/fail?

Yes.

Have you run the entire test suite and no regression was detected?

Yes. It fails locally but on just one unrelated test (sliceVariation.t) which is due the MySQL version I have (5.7) which is currently incompatible with the Variation API. The very same fail occurs also on master.

…ary searches in mappers.

…ntered interval tree, to be used in the mapper instead of binary search.

…harmonise with mapper code.

…arch.

…mapped coordinates. More uniform function prototype to allow transparent calls from Slice project method.

… using the values returned by invoking the updated mapper interfaces which return the original and mapped coordinates. The coordinates are readjusted consider the slice strand. There are some minor optimisations which avoid going through unnecessary code fragments in certain circumnstances.

…per.

…sponding seq region names.

…rval trees. It loads XS version if it's installed, otherwise fall back to PP implementation.

…ype.

… insert method.

magaliruffier · 2019-01-14T13:27:20Z

This still needs to be resolved, with feedback from the Infrastructure team and updates from Alessandro

mkszuba

Sadly, needs quite a bit more work. In addition to my comments inline, isn't there some sort of generic tree implementation we could use instead of rolling our own?

mkszuba · 2019-01-26T14:54:20Z

modules/Bio/EnsEMBL/AssemblyMapper.pm

@@ -224,7 +224,7 @@ sub map {
  throw('Incorrect number of arguments.') if (!( @_ >= 6));

  my ( $self, $frm_seq_region_name, $frm_start, $frm_end, $frm_strand,
-       $frm_cs, $to_slice )
+       $frm_cs, $dummy, $to_slice, $include_org_coord )


Please update the argument list in POD accordingly. If possible, fix the things which were out of date before the change as well.

mkszuba · 2019-01-26T14:59:11Z

modules/Bio/EnsEMBL/ChainedAssemblyMapper.pm

@@ -277,7 +277,7 @@ sub map {
  throw('Incorrect number of arguments.') if(@_ < 6);

  my ($self, $frm_seq_region_name, $frm_start,
-      $frm_end, $frm_strand, $frm_cs, $fastmap, $to_slice) = @_;
+      $frm_end, $frm_strand, $frm_cs, $fastmap, $to_slice, $include_org_coord) = @_;


mkszuba · 2019-01-26T14:59:53Z

modules/Bio/EnsEMBL/Mapper.pm

@@ -149,6 +149,9 @@ use Bio::EnsEMBL::Mapper::IndelCoordinate;
 use Bio::EnsEMBL::Mapper::Gap;

 use Bio::EnsEMBL::Utils::Exception qw(throw);
+use Bio::EnsEMBL::Utils::Interval;
+use Bio::EnsEMBL::Utils::Tree::Interval::Immutable;
+# use Bio::EnsEMBL::Utils::Tree::Interval::Mutable;


Please remove commented-out code.

mkszuba · 2019-01-26T18:02:33Z

modules/Bio/EnsEMBL/Mapper.pm

+
+  ###
+  #
+  map { push @{$from_intervals},


I am not a fan of using map in void context and writing output inline.

mkszuba · 2019-01-26T18:03:18Z

modules/Bio/EnsEMBL/Mapper.pm

+  # Enable this as some preliminary experiments show this is slightly faster
+  # on the test suite.
+  #
+  # my $itree = Bio::EnsEMBL::Utils::Tree::Interval::Mutable->new();


Commented-out code.

mkszuba · 2019-01-28T15:57:01Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable.pm

+
+Bio::EnsEMBL::Utils::Tree::Interval::Mutable
+
+=head1 SYNOPSIS


And more...

(still here)

mkszuba · 2019-01-28T15:58:42Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable/Node.pm

+
+=head1 DESCRIPTION
+
+=head1 METHODS


And more...

mkszuba · 2019-01-28T16:00:29Z

modules/t/interval_tree_mutable.t

+use Test::Exception;
+use Test::Warnings qw(warning);
+
+use Data::Dumper;


This module isn't used anywhere in this file, is it?

mkszuba · 2019-01-28T16:00:35Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Immutable.pm

+
+use strict;
+
+use Data::Dumper;


This module isn't used anywhere in this file, is it?

mkszuba · 2019-01-28T16:00:46Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable.pm

+
+use strict;
+
+use Data::Dumper;


This module isn't used anywhere in this file, is it?

(still here)

…comments and in-place conditionals.

mkszuba

I've just flagged two previously requested changes that have been missed. Other than those two, fine by me.

mkszuba · 2019-01-29T16:04:18Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable.pm

+
+Bio::EnsEMBL::Utils::Tree::Interval::Mutable
+
+=head1 SYNOPSIS


(still here)

mkszuba · 2019-01-29T16:04:33Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable.pm

+
+use strict;
+
+use Data::Dumper;


(still here)

tgrego · 2019-02-08T15:13:04Z

@avullo can you please merge master into your branch and push it just so we can make sure travis is happy with it all?
thanks

avullo · 2019-02-08T15:24:10Z

@tgrego just done

modules/t/interval_tree_mutable.t

markmcdowall · 2019-02-18T09:43:00Z

modules/Bio/EnsEMBL/Mapper.pm

-  $start_idx = 0;
-  $end_idx   = $#{$lr};
+  foreach my $i (@{$lr}) {
+    my $start = $i->{$from}{start}<=$i->{$from}{end}?$i->{$from}{start}:$i->{$from}{end};


This isn't the clearest. Could this and the line below get split into an single if. The $start and $end can get created as $i->{$from}{start} and $i->{$from}{end} respectively then modified if they fail the if condition. This would also reduce the checks

Thanks Mark, this is addressed now.

That look much better, thank you

The points against using an off the shelf implementation:

minimise reliance on external libraries (i.e. Set::IntervalTree)

Tree::Interval only handles the non-overlapping intervals case

ability to provide optimize (i.e. XS) code, when possible

ability to provide different suitable implementations (i.e. mutable/immutable) depending on the use case

markmcdowall

Other than some minor clarity changes and some more docs, this is looking good.

I'd like to reiterate @mkszuba question about the implementation of tree interval code and why we could not use an off the shelf implementation.

markmcdowall · 2019-02-18T09:53:02Z

modules/Bio/EnsEMBL/Slice.pm

-						   $normal_slice->end());
-    }
+    my @coords = $asm_mapper->map($normal_slice->seq_region_name(),
+				  $normal_slice->start(),


Please remove all tabs

markmcdowall · 2019-02-18T10:49:39Z

modules/Bio/EnsEMBL/Slice.pm

-						   $normal_slice->end());
-    }
+    my @coords = $asm_mapper->map($normal_slice->seq_region_name(),
+				  $normal_slice->start(),


Please remove the tabs

markmcdowall · 2019-02-18T10:51:12Z

modules/Bio/EnsEMBL/TopLevelAssemblyMapper.pm

+        if(@coords > 1) {
+	  return @coords;
+	} elsif ($include_org_coord) {
+	  return @coords unless $coords[0]{mapped}->isa('Bio::EnsEMBL::Mapper::Gap');


Can the unless get moved to the elsif block

markmcdowall · 2019-02-18T10:52:22Z

modules/Bio/EnsEMBL/TopLevelAssemblyMapper.pm

+	} elsif ($include_org_coord) {
+	  return @coords unless $coords[0]{mapped}->isa('Bio::EnsEMBL::Mapper::Gap');
+	} else {
+	  return @coords unless $coords[0]->isa('Bio::EnsEMBL::Mapper::Gap');


Could this not get handled as part of the if rather than a postfix in the else section?

markmcdowall · 2019-02-18T10:55:53Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Immutable.pm

+  return sort_by_begin(uniq($result));
+}
+
+sub _query_point {


Where is the pod section?

markmcdowall · 2019-02-18T11:14:41Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable.pm

+  _load($PP);
+}
+
+sub _load {


markmcdowall · 2019-02-18T11:18:55Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable/Node.pm

+  my $left = $self->left;
+
+  $left->parent($self->parent);
+  unless (defined $left->parent) {


Could this not be swapped so the if (defined $right->parent) then perform what is in the else block

markmcdowall · 2019-02-18T11:19:13Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable/Node.pm

+  my $right = $self->right;
+
+  $right->parent($self->parent);
+  unless (defined $right->parent) {


Could this not be swapped so the if (defined $right->parent) then perform what is in the else block

markmcdowall · 2019-02-18T11:19:44Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable/Node.pm

+    }
+  } else {
+    # insert into right subtree
+    unless (defined $self->right) {


Could this not be swapped so the if (defined $self->right) then perform what is in the else block otherwise$self->right(Bio::EnsEMBL::Utils...

markmcdowall · 2019-02-18T11:19:48Z

modules/Bio/EnsEMBL/Utils/Tree/Interval/Mutable/Node.pm

+
+  if ($i->start < $self->key) {
+    # insert into left subtree
+    unless (defined $self->left) {


Could this not be swapped so the if (defined $self->left) then perform what is in the else block otherwise$self->left(Bio::EnsEMBL::Utils...

tgrego · 2019-03-08T14:55:36Z

Looks like no more changes will be added.
As there are no blockers, I will be merging later today unless I hear something against it.

avullo · 2019-03-08T15:01:00Z

Pardon I don't have time to address the latest comments. They appear to be mostly stylistic in nature, something which can later be added. Moreover, POD's missing only for the private methods, which was intentional.

Alessandro Vullo added 30 commits November 13, 2018 16:19

[ENSCORESW-2168]. Initial tests for the candidate replacements of bin…

cd37bc9

…ary searches in mappers.

[ENSCORESW-2168]. Initial prototype implementation of an immutable ce…

e8ea919

…ntered interval tree, to be used in the mapper instead of binary search.

[ENSCORESW-2168]. Additional tests to cover design extensions.

1e7ef5c

[ENSCORESW-2168]. Support for efficient query of points and intervals.

0699d73

[ENSCORESW-2168]. Extended API to support design improvements.

c38fb4f

[ENSCORESW-2168]. Test using Interval Tree.

799ea43

[ENSCORESW-2168]. Return query results sorted by start coordinate to …

5ed26ca

…harmonise with mapper code.

[ENSCORESW-2168]. Must allow zero coordinates but not undefined ones.

85ee658

[ENSCORESW-2168]. There can be zero coordinates.

8f868f6

[ENSCORESW-2168]. Simplified API, allow passing only boundaries.

03f05aa

[ENSCORESW-2168]. Use simplified interval tree API, discard binary se…

3be3413

…arch.

[ENSCORESW-2168]. Use interval tree API, discard binary search.

8048e69

[ENSCORESW-2168]. Consider the indel case.

6c288b6

[ENSCORESW-2289]. Call underlying mapper code to return original and …

9db6e3e

…mapped coordinates. More uniform function prototype to allow transparent calls from Slice project method.

[ENSCORESW-2289]. Consider necessary option for Slice::project.

1f2ab70

[ENSCORESW-2289]. Match signature of ChainedAssemblyMapper.

dfb23c2

[ENSCORESW-2289]. Consider returning original coordinates in all cases.

7779b42

[ENSCORESW-844]. Test mapped coordinate names for ChainedAssemblyMapper.

9a12e6f

[ENSCORESW-844]. Test mapped coordinate names for TopLevelAssemblyMap…

a4a9bac

…per.

[ENSCORESW-844]. Mapper decorates returned coordinates with the corre…

de8ee24

…sponding seq region names.

[ENSCORESW-844]. Mapper decorates returned coordinates with the corre…

61065d3

…sponding seq region names.

[ENSCORESW-2541]. Wrapper around XS/PP implementation of mutable inte…

98ffc13

…rval trees. It loads XS version if it's installed, otherwise fall back to PP implementation.

[ENSCORESW-2541]. PP implementation of mutable interval trees, protot…

ec8382e

…ype.

[ENSCORESW-2541]. Prototype unit test for mutable interval trees.

44a03f8

[ENSCORESW-2541]. Too much ahead, revert to current year.

0a381c5

[ENSCORESW-2541]. This one too was too much in the future.

57e6caa

[ENSCORESW-2541]. Some relevant documentation.

4350430

[ENSCORESW-2541]. This one too was in the future.

0685a61

[ENSCORESW-2541]. Tree needs root node. Initial implementation of the…

9d87fbc

… insert method.

magaliruffier requested a review from mkszuba January 21, 2019 17:10

mkszuba suggested changes Jan 28, 2019

View reviewed changes

Alessandro Vullo added 9 commits January 29, 2019 11:14

[ENSCORESW-2168]. Remove commented code and (weird) formatting.

09037ac

[ENSCORESW-2168]. Avoid map use in void context.

b41ab42

Commented code.

9299d86

POD udpate.

d0fdbdf

Back to (others) preferred mode.

97445a8

Complete POD.

58b1041

[ENSCORESW-2168]. Avoid postfix notation.

721feb4

[ENSCORESW-2168]. Added POD, remove unused dependencies, unnecessary …

0e1ce9f

…comments and in-place conditionals.

[ENSCORESW-2168]. POD and rearragend dependencies.

9fbbb96

mkszuba suggested changes Jan 29, 2019

View reviewed changes

[ENSCORESW-2168]. Adde POD and remove unused dependencies.

6f1041d

mkszuba approved these changes Jan 29, 2019

View reviewed changes

tgrego approved these changes Jan 30, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master'

41ec50e

ens-bwalts suggested changes Feb 12, 2019

View reviewed changes

modules/t/interval_tree_mutable.t Outdated Show resolved Hide resolved

modules/t/interval_tree_mutable.t Outdated Show resolved Hide resolved

modules/t/interval_tree_mutable.t Show resolved Hide resolved

Alessandro Vullo added 2 commits February 13, 2019 14:59

[ENSCORESW-2168]. Test negative search + remove unused subroutine.

12b4726

[ENSCORESW-2168]. Correct Copyright year. Happy travis again.

972e286

markmcdowall reviewed Feb 18, 2019

View reviewed changes

[ENSCORESW-2168]. Cleaner implementation, less comparisons.

c0b7457

ens-bwalts approved these changes Feb 18, 2019

View reviewed changes

markmcdowall reviewed Feb 18, 2019

View reviewed changes

tgrego merged commit ee62359 into Ensembl:master Mar 8, 2019

ens-bwalts mentioned this pull request Jun 17, 2019

ENSCORESW-3132: Limited implementation of Intervals with start > end … #393

Merged


		Bio::EnsEMBL::Utils::Tree::Interval::Mutable

		=head1 SYNOPSIS


		=head1 DESCRIPTION

		=head1 METHODS


		Bio::EnsEMBL::Utils::Tree::Interval::Mutable

		=head1 SYNOPSIS

Necessary Updates to the Mapper code [ENSCORESW-2168]. #332

Necessary Updates to the Mapper code [ENSCORESW-2168]. #332

Conversation

avullo commented Nov 13, 2018

Requirements

Description

Use case

Benefits

Possible Drawbacks

Testing

magaliruffier commented Jan 14, 2019

mkszuba 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

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

Choose a reason for hiding this comment

mkszuba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgrego commented Feb 8, 2019

avullo commented Feb 8, 2019

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

markmcdowall 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

Choose a reason for hiding this comment

tgrego commented Mar 8, 2019

avullo commented Mar 8, 2019