JAL-2089 Merge branch releases/Release_2_10_Branch to master
[jalview.git] / src / jalview / analysis / CrossRef.java
index d96ab58..23308d6 100644 (file)
@@ -1,6 +1,6 @@
 /*
- * Jalview - A Sequence Alignment Editor and Viewer ($$Version-Rel$$)
- * Copyright (C) $$Year-Rel$$ The Jalview Authors
+ * Jalview - A Sequence Alignment Editor and Viewer (Version 2.9.0b2)
+ * Copyright (C) 2015 The Jalview Authors
  * 
  * This file is part of Jalview.
  * 
@@ -24,11 +24,11 @@ import jalview.datamodel.AlignedCodonFrame;
 import jalview.datamodel.Alignment;
 import jalview.datamodel.AlignmentI;
 import jalview.datamodel.DBRefEntry;
+import jalview.datamodel.DBRefSource;
 import jalview.datamodel.Mapping;
 import jalview.datamodel.Sequence;
 import jalview.datamodel.SequenceFeature;
 import jalview.datamodel.SequenceI;
-import jalview.util.Comparison;
 import jalview.util.DBRefUtils;
 import jalview.util.MapList;
 import jalview.ws.SequenceFetcherFactory;
@@ -59,6 +59,16 @@ public class CrossRef
   private SequenceI[] fromSeqs;
 
   /**
+   * matcher built from dataset
+   */
+  SequenceIdMatcher matcher;
+
+  /**
+   * sequences found by cross-ref searches to fromSeqs
+   */
+  List<SequenceI> rseqs;
+
+  /**
    * Constructor
    * 
    * @param seqs
@@ -97,6 +107,16 @@ public class CrossRef
         findXrefSourcesForSequence(seq, dna, sources);
       }
     }
+    sources.remove(DBRefSource.EMBL); // hack to prevent EMBL xrefs resulting in
+                                      // redundant datasets
+    if (dna)
+    {
+      sources.remove(DBRefSource.ENSEMBL); // hack to prevent Ensembl and
+                                           // EnsemblGenomes xref option shown
+                                           // from cdna panel
+      sources.remove(DBRefSource.ENSEMBLGENOMES);
+    }
+    // redundant datasets
     return sources;
   }
 
@@ -111,6 +131,9 @@ public class CrossRef
    * 
    * @param seq
    *          the sequence whose dbrefs we are searching against
+   * @param fromDna
+   *          when true, context is DNA - so sources identifying protein
+   *          products will be returned.
    * @param sources
    *          a list of sources to add matches to
    */
@@ -128,18 +151,18 @@ public class CrossRef
        * find sequence's direct (dna-to-dna, peptide-to-peptide) xrefs
        */
       DBRefEntry[] lrfs = DBRefUtils.selectDbRefs(fromDna, seq.getDBRefs());
-      List<SequenceI> rseqs = new ArrayList<SequenceI>();
+      List<SequenceI> foundSeqs = new ArrayList<SequenceI>();
 
       /*
        * find sequences in the alignment which xref one of these DBRefs
        * i.e. is xref-ed to a common sequence identifier
        */
-      searchDatasetXrefs(fromDna, seq, lrfs, rseqs, null);
+      searchDatasetXrefs(fromDna, seq, lrfs, foundSeqs, null);
 
       /*
        * add those sequences' (dna-to-peptide or peptide-to-dna) dbref sources
        */
-      for (SequenceI rs : rseqs)
+      for (SequenceI rs : foundSeqs)
       {
         DBRefEntry[] xrs = DBRefUtils
                 .selectDbRefs(!fromDna, rs.getDBRefs());
@@ -195,10 +218,9 @@ public class CrossRef
   public Alignment findXrefSequences(String source, boolean fromDna)
   {
 
-    List<SequenceI> rseqs = new ArrayList<SequenceI>();
+    rseqs = new ArrayList<SequenceI>();
     AlignedCodonFrame cf = new AlignedCodonFrame();
-    SequenceIdMatcher matcher = new SequenceIdMatcher(
-            dataset.getSequences());
+    matcher = new SequenceIdMatcher(dataset.getSequences());
 
     for (SequenceI seq : fromSeqs)
     {
@@ -210,6 +232,9 @@ public class CrossRef
       boolean found = false;
       DBRefEntry[] xrfs = DBRefUtils
               .selectDbRefs(!fromDna, dss.getDBRefs());
+      // ENST & ENSP comes in to both Protein and nucleotide, so we need to
+      // filter them
+      // out later.
       if ((xrfs == null || xrfs.length == 0) && dataset != null)
       {
         /*
@@ -237,11 +262,15 @@ public class CrossRef
       List<DBRefEntry> sourceRefs = DBRefUtils.searchRefsForSource(xrfs,
               source);
       Iterator<DBRefEntry> refIterator = sourceRefs.iterator();
+      // At this point, if we are retrieving Ensembl, we still don't filter out
+      // ENST when looking for protein crossrefs.
       while (refIterator.hasNext())
       {
         DBRefEntry xref = refIterator.next();
         found = false;
-        if (xref.hasMap())
+        // we're only interested in coding cross-references, not
+        // locus->transcript
+        if (xref.hasMap() && xref.getMap().getMap().isTripletMap())
         {
           SequenceI mappedTo = xref.getMap().getTo();
           if (mappedTo != null)
@@ -259,6 +288,18 @@ public class CrossRef
              * but findInDataset() matches ENSP when looking for Uniprot...
              */
             SequenceI matchInDataset = findInDataset(xref);
+            if (matchInDataset != null && xref.getMap().getTo() != null
+                    && matchInDataset != xref.getMap().getTo())
+            {
+              System.err
+                      .println("Implementation problem (reopen JAL-2154): CrossRef.findInDataset seems to have recovered a different sequence than the one explicitly mapped for xref."
+                              + "Found:"
+                              + matchInDataset
+                              + "\nExpected:"
+                              + xref.getMap().getTo()
+                              + "\nFor xref:"
+                              + xref);
+            }
             /*matcher.findIdMatch(mappedTo);*/
             if (matchInDataset != null)
             {
@@ -266,24 +307,47 @@ public class CrossRef
               {
                 rseqs.add(matchInDataset);
               }
+              // even if rseqs contained matchInDataset - check mappings between
+              // these seqs are added
+              // need to try harder to only add unique mappings
+              if (xref.getMap().getMap().isTripletMap()
+                      && dataset.getMapping(seq, matchInDataset) == null
+                      && cf.getMappingBetween(seq, matchInDataset) == null)
+              {
+                // materialise a mapping for highlighting between these
+                // sequences
+                if (fromDna)
+                {
+                  cf.addMap(dss, matchInDataset, xref.getMap().getMap(),
+                          xref.getMap().getMappedFromId());
+                }
+                else
+                {
+                  cf.addMap(matchInDataset, dss, xref.getMap().getMap()
+                          .getInverse(), xref.getMap().getMappedFromId());
+                }
+              }
+
               refIterator.remove();
               continue;
             }
+            // TODO: need to determine if this should be a deriveSequence
             SequenceI rsq = new Sequence(mappedTo);
             rseqs.add(rsq);
-            if (xref.getMap().getMap().getFromRatio() != xref.getMap()
-                    .getMap().getToRatio())
+            if (xref.getMap().getMap().isTripletMap())
             {
               // get sense of map correct for adding to product alignment.
               if (fromDna)
               {
                 // map is from dna seq to a protein product
-                cf.addMap(dss, rsq, xref.getMap().getMap());
+                cf.addMap(dss, rsq, xref.getMap().getMap(), xref.getMap()
+                        .getMappedFromId());
               }
               else
               {
                 // map should be from protein seq to its coding dna
-                cf.addMap(rsq, dss, xref.getMap().getMap().getInverse());
+                cf.addMap(rsq, dss, xref.getMap().getMap().getInverse(),
+                        xref.getMap().getMappedFromId());
               }
             }
           }
@@ -293,7 +357,9 @@ public class CrossRef
         {
           SequenceI matchedSeq = matcher.findIdMatch(xref.getSource() + "|"
                   + xref.getAccessionId());
-          if (matchedSeq != null)
+          // if there was a match, check it's at least the right type of
+          // molecule!
+          if (matchedSeq != null && matchedSeq.isProtein() == fromDna)
           {
             if (constructMapping(seq, matchedSeq, xref, cf, fromDna))
             {
@@ -319,151 +385,285 @@ public class CrossRef
        */
       if (!sourceRefs.isEmpty())
       {
-        ASequenceFetcher sftch = SequenceFetcherFactory
-                .getSequenceFetcher();
-        SequenceI[] retrieved = null;
-        try
+        retrieveCrossRef(sourceRefs, seq, xrfs, fromDna, cf);
+      }
+    }
+
+    Alignment ral = null;
+    if (rseqs.size() > 0)
+    {
+      ral = new Alignment(rseqs.toArray(new SequenceI[rseqs.size()]));
+      if (!cf.isEmpty())
+      {
+        dataset.addCodonFrame(cf);
+      }
+    }
+    return ral;
+  }
+
+  private void retrieveCrossRef(List<DBRefEntry> sourceRefs, SequenceI seq,
+          DBRefEntry[] xrfs, boolean fromDna, AlignedCodonFrame cf)
+  {
+    ASequenceFetcher sftch = SequenceFetcherFactory.getSequenceFetcher();
+    SequenceI[] retrieved = null;
+    SequenceI dss = seq.getDatasetSequence() == null ? seq : seq
+            .getDatasetSequence();
+    // first filter in case we are retrieving crossrefs that have already been
+    // retrieved. this happens for cases where a database record doesn't yield
+    // protein products for CDS
+    removeAlreadyRetrievedSeqs(sourceRefs, fromDna);
+    if (sourceRefs.size() == 0)
+    {
+      // no more work to do! We already had all requested sequence records in
+      // the dataset.
+      return;
+    }
+    try
+    {
+      retrieved = sftch.getSequences(sourceRefs, !fromDna);
+    } catch (Exception e)
+    {
+      System.err
+              .println("Problem whilst retrieving cross references for Sequence : "
+                      + seq.getName());
+      e.printStackTrace();
+    }
+
+    if (retrieved != null)
+    {
+      boolean addedXref = false;
+      List<SequenceI> newDsSeqs = new ArrayList<SequenceI>(), doNotAdd = new ArrayList<SequenceI>();
+
+      for (SequenceI retrievedSequence : retrieved)
+      {
+        // dataset gets contaminated ccwith non-ds sequences. why ??!
+        // try: Ensembl -> Nuc->Ensembl, Nuc->Uniprot-->Protein->EMBL->
+        SequenceI retrievedDss = retrievedSequence.getDatasetSequence() == null ? retrievedSequence
+                : retrievedSequence.getDatasetSequence();
+        addedXref |= importCrossRefSeq(cf, newDsSeqs, doNotAdd, dss,
+                retrievedDss);
+      }
+      if (!addedXref)
+      {
+        // try again, after looking for matching IDs
+        // shouldn't need to do this unless the dbref mechanism has broken.
+        updateDbrefMappings(seq, xrfs, retrieved, cf, fromDna);
+        for (SequenceI retrievedSequence : retrieved)
         {
-          retrieved = sftch.getSequences(sourceRefs, !fromDna);
-        } catch (Exception e)
+          // dataset gets contaminated ccwith non-ds sequences. why ??!
+          // try: Ensembl -> Nuc->Ensembl, Nuc->Uniprot-->Protein->EMBL->
+          SequenceI retrievedDss = retrievedSequence.getDatasetSequence() == null ? retrievedSequence
+                  : retrievedSequence.getDatasetSequence();
+          addedXref |= importCrossRefSeq(cf, newDsSeqs, doNotAdd, dss,
+                  retrievedDss);
+        }
+      }
+      for (SequenceI newToSeq : newDsSeqs)
+      {
+        if (!doNotAdd.contains(newToSeq)
+                && dataset.findIndex(newToSeq) == -1)
         {
-          System.err
-                  .println("Problem whilst retrieving cross references for Sequence : "
-                          + seq.getName());
-          e.printStackTrace();
+          dataset.addSequence(newToSeq);
+          matcher.add(newToSeq);
         }
+      }
+    }
+  }
 
-        if (retrieved != null)
+  /**
+   * Search dataset for sequences with a primary reference contained in
+   * sourceRefs.
+   * 
+   * @param sourceRefs
+   *          - list of references to filter.
+   * @param fromDna
+   *          - type of sequence to search for matching primary reference.
+   */
+  private void removeAlreadyRetrievedSeqs(List<DBRefEntry> sourceRefs,
+          boolean fromDna)
+  {
+    DBRefEntry[] dbrSourceSet = sourceRefs.toArray(new DBRefEntry[0]);
+    for (SequenceI sq : dataset.getSequences())
+    {
+      boolean dupeFound = false;
+      // !fromDna means we are looking only for nucleotide sequences, not
+      // protein
+      if (sq.isProtein() == fromDna)
+      {
+        for (DBRefEntry dbr : sq.getPrimaryDBRefs())
+        {
+          for (DBRefEntry found : DBRefUtils.searchRefs(dbrSourceSet, dbr))
+          {
+            sourceRefs.remove(found);
+            dupeFound = true;
+          }
+        }
+      }
+      if (dupeFound)
+      {
+        // rebuild the search array from the filtered sourceRefs list
+        dbrSourceSet = sourceRefs.toArray(new DBRefEntry[0]);
+      }
+    }
+  }
+
+  /**
+   * process sequence retrieved via a dbref on source sequence to resolve and
+   * transfer data
+   * 
+   * @param cf
+   * @param sourceSequence
+   * @param retrievedSequence
+   * @return true if retrieveSequence was imported
+   */
+  private boolean importCrossRefSeq(AlignedCodonFrame cf,
+          List<SequenceI> newDsSeqs, List<SequenceI> doNotAdd,
+          SequenceI sourceSequence, SequenceI retrievedSequence)
+  {
+    /**
+     * set when retrievedSequence has been verified as a crossreference for
+     * sourceSequence
+     */
+    boolean imported = false;
+    DBRefEntry[] dbr = retrievedSequence.getDBRefs();
+    if (dbr != null)
+    {
+      for (DBRefEntry dbref : dbr)
+      {
+        SequenceI matched = findInDataset(dbref);
+        if (matched == sourceSequence)
+        {
+          // verified retrieved and source sequence cross-reference each other
+          imported = true;
+        }
+        // find any entry where we should put in the sequence being
+        // cross-referenced into the map
+        Mapping map = dbref.getMap();
+        if (map != null)
         {
-          updateDbrefMappings(seq, xrfs, retrieved, cf, fromDna);
-          for (SequenceI retrievedSequence : retrieved)
+          if (map.getTo() != null && map.getMap() != null)
           {
-            SequenceI retrievedDss = retrievedSequence.getDatasetSequence() == null ? retrievedSequence
-                    : retrievedSequence.getDatasetSequence();
-            DBRefEntry[] dbr = retrievedSequence.getDBRefs();
-            if (dbr != null)
+            if (map.getTo() == sourceSequence)
             {
-              for (DBRefEntry dbref : dbr)
+              // already called to import once, and most likely this sequence
+              // already imported !
+              continue;
+            }
+            if (matched == null)
+            {
+              /*
+               * sequence is new to dataset, so save a reference so it can be added. 
+               */
+              newDsSeqs.add(map.getTo());
+              continue;
+            }
+
+            /*
+             * there was a matching sequence in dataset, so now, check to see if we can update the map.getTo() sequence to the existing one.
+             */
+
+            try
+            {
+              // compare ms with dss and replace with dss in mapping
+              // if map is congruent
+              SequenceI ms = map.getTo();
+              // TODO findInDataset requires exact sequence match but
+              // 'congruent' test is only for the mapped part
+              // maybe not a problem in practice since only ENA provide a
+              // mapping and it is to the full protein translation of CDS
+              // matcher.findIdMatch(map.getTo());
+              // TODO addendum: if matched is shorter than getTo, this will fail
+              // - when it should really succeed.
+              int sf = map.getMap().getToLowest();
+              int st = map.getMap().getToHighest();
+              SequenceI mappedrg = ms.getSubSequence(sf, st);
+              if (mappedrg.getLength() > 0
+                      && ms.getSequenceAsString().equals(
+                              matched.getSequenceAsString()))
               {
-                // find any entry where we should put in the sequence being
-                // cross-referenced into the map
-                Mapping map = dbref.getMap();
-                if (map != null)
+                /*
+                 * sequences were a match, 
+                 */
+                String msg = "Mapping updated from " + ms.getName()
+                        + " to retrieved crossreference "
+                        + matched.getName();
+                System.out.println(msg);
+
+                DBRefEntry[] toRefs = map.getTo().getDBRefs();
+                if (toRefs != null)
                 {
-                  if (map.getTo() != null && map.getMap() != null)
+                  /*
+                   * transfer database refs
+                   */
+                  for (DBRefEntry ref : toRefs)
                   {
-                    // TODO findInDataset requires exact sequence match but
-                    // 'congruent' test is only for the mapped part
-                    // maybe not a problem in practice since only ENA provide a
-                    // mapping and it is to the full protein translation of CDS
-                    SequenceI matched = findInDataset(dbref);
-                    // matcher.findIdMatch(map.getTo());
-                    if (matched != null)
+                    if (dbref.getSrcAccString().equals(
+                            ref.getSrcAccString()))
                     {
-                      /*
-                       * already got an xref to this sequence; update this
-                       * map to point to the same sequence, and add
-                       * any new dbrefs to it
-                       */
-                      DBRefEntry[] toRefs = map.getTo().getDBRefs();
-                      if (toRefs != null)
-                      {
-                        for (DBRefEntry ref : toRefs)
-                        {
-                          matched.addDBRef(ref); // add or update mapping
-                        }
-                      }
-                      map.setTo(matched);
+                      continue; // avoid overwriting the ref on source sequence
                     }
-                    else
-                    {
-                      matcher.add(map.getTo());
-                    }
-                    try
+                    matched.addDBRef(ref); // add or update mapping
+                  }
+                }
+                doNotAdd.add(map.getTo());
+                map.setTo(matched);
+
+                /*
+                 * give the reverse reference the inverse mapping 
+                 * (if it doesn't have one already)
+                 */
+                setReverseMapping(matched, dbref, cf);
+
+                /*
+                 * copy sequence features as well, avoiding
+                 * duplication (e.g. same variation from two 
+                 * transcripts)
+                 */
+                SequenceFeature[] sfs = ms.getSequenceFeatures();
+                if (sfs != null)
+                {
+                  for (SequenceFeature feat : sfs)
+                  {
+                    /*
+                     * make a flyweight feature object which ignores Parent
+                     * attribute in equality test; this avoids creating many
+                     * otherwise duplicate exon features on genomic sequence
+                     */
+                    SequenceFeature newFeature = new SequenceFeature(feat)
                     {
-                      // compare ms with dss and replace with dss in mapping
-                      // if map is congruent
-                      SequenceI ms = map.getTo();
-                      int sf = map.getMap().getToLowest();
-                      int st = map.getMap().getToHighest();
-                      SequenceI mappedrg = ms.getSubSequence(sf, st);
-                      // SequenceI loc = dss.getSubSequence(sf, st);
-                      if (mappedrg.getLength() > 0
-                              && ms.getSequenceAsString().equals(
-                                      dss.getSequenceAsString()))
-                      // && mappedrg.getSequenceAsString().equals(
-                      // loc.getSequenceAsString()))
+                      @Override
+                      public boolean equals(Object o)
                       {
-                        String msg = "Mapping updated from " + ms.getName()
-                                + " to retrieved crossreference "
-                                + dss.getName();
-                        System.out.println(msg);
-                        map.setTo(dss);
-
-                        /*
-                         * give the reverse reference the inverse mapping 
-                         * (if it doesn't have one already)
-                         */
-                        setReverseMapping(dss, dbref, cf);
-
-                        /*
-                         * copy sequence features as well, avoiding
-                         * duplication (e.g. same variation from two 
-                         * transcripts)
-                         */
-                        SequenceFeature[] sfs = ms.getSequenceFeatures();
-                        if (sfs != null)
-                        {
-                          for (SequenceFeature feat : sfs)
-                          {
-                            /*
-                             * make a flyweight feature object which ignores Parent
-                             * attribute in equality test; this avoids creating many
-                             * otherwise duplicate exon features on genomic sequence
-                             */
-                            SequenceFeature newFeature = new SequenceFeature(
-                                    feat)
-                            {
-                              @Override
-                              public boolean equals(Object o)
-                              {
-                                return super.equals(o, true);
-                              }
-                            };
-                            dss.addSequenceFeature(newFeature);
-                          }
-                        }
+                        return super.equals(o, true);
                       }
-                      cf.addMap(retrievedDss, map.getTo(), map.getMap());
-                    } catch (Exception e)
-                    {
-                      System.err
-                              .println("Exception when consolidating Mapped sequence set...");
-                      e.printStackTrace(System.err);
-                    }
+                    };
+                    matched.addSequenceFeature(newFeature);
                   }
                 }
+
               }
+              cf.addMap(retrievedSequence, map.getTo(), map.getMap());
+            } catch (Exception e)
+            {
+              System.err
+                      .println("Exception when consolidating Mapped sequence set...");
+              e.printStackTrace(System.err);
             }
-            retrievedSequence.updatePDBIds();
-            rseqs.add(retrievedDss);
-            dataset.addSequence(retrievedDss);
-            matcher.add(retrievedDss);
           }
         }
       }
     }
-
-    Alignment ral = null;
-    if (rseqs.size() > 0)
+    if (imported)
     {
-      ral = new Alignment(rseqs.toArray(new SequenceI[rseqs.size()]));
-      if (!cf.isEmpty())
+      retrievedSequence.updatePDBIds();
+      rseqs.add(retrievedSequence);
+      if (dataset.findIndex(retrievedSequence) == -1)
       {
-        dataset.addCodonFrame(cf);
+        dataset.addSequence(retrievedSequence);
+        matcher.add(retrievedSequence);
       }
     }
-    return ral;
+    return imported;
   }
 
   /**
@@ -508,9 +708,12 @@ public class CrossRef
   }
 
   /**
-   * Returns the first identical sequence in the dataset if any, else null
+   * Returns null or the first sequence in the dataset which is identical to
+   * xref.mapTo, and has a) a primary dbref matching xref, or if none found, the
+   * first one with an ID source|xrefacc
    * 
    * @param xref
+   *          with map and mapped-to sequence
    * @return
    */
   SequenceI findInDataset(DBRefEntry xref)
@@ -524,22 +727,42 @@ public class CrossRef
     String name2 = xref.getSource() + "|" + name;
     SequenceI dss = mapsTo.getDatasetSequence() == null ? mapsTo : mapsTo
             .getDatasetSequence();
+    // first check ds if ds is directly referenced
+    if (dataset.findIndex(dss) > -1)
+    {
+      return dss;
+    }
+    DBRefEntry template = new DBRefEntry(xref.getSource(), null,
+            xref.getAccessionId());
+    /**
+     * remember the first ID match - in case we don't find a match to template
+     */
+    SequenceI firstIdMatch = null;
     for (SequenceI seq : dataset.getSequences())
     {
+      // first check primary refs.
+      List<DBRefEntry> match = DBRefUtils.searchRefs(seq.getPrimaryDBRefs()
+              .toArray(new DBRefEntry[0]), template);
+      if (match != null && match.size() == 1 && sameSequence(seq, dss))
+      {
+        return seq;
+      }
       /*
        * clumsy alternative to using SequenceIdMatcher which currently
        * returns sequences with a dbref to the matched accession id 
        * which we don't want
        */
-      if (name.equals(seq.getName()) || seq.getName().startsWith(name2))
+      if (firstIdMatch == null
+              && (name.equals(seq.getName()) || seq.getName().startsWith(
+                      name2)))
       {
         if (sameSequence(seq, dss))
         {
-          return seq;
+          firstIdMatch = seq;
         }
       }
     }
-    return null;
+    return firstIdMatch;
   }
 
   /**
@@ -596,14 +819,14 @@ public class CrossRef
   void updateDbrefMappings(SequenceI mapFrom, DBRefEntry[] xrefs,
           SequenceI[] retrieved, AlignedCodonFrame acf, boolean fromDna)
   {
-    SequenceIdMatcher matcher = new SequenceIdMatcher(retrieved);
+    SequenceIdMatcher idMatcher = new SequenceIdMatcher(retrieved);
     for (DBRefEntry xref : xrefs)
     {
       if (!xref.hasMap())
       {
         String targetSeqName = xref.getSource() + "|"
                 + xref.getAccessionId();
-        SequenceI[] matches = matcher.findAllIdMatches(targetSeqName);
+        SequenceI[] matches = idMatcher.findAllIdMatches(targetSeqName);
         if (matches == null)
         {
           return;
@@ -640,24 +863,28 @@ public class CrossRef
           DBRefEntry xref, AlignedCodonFrame mappings, boolean fromDna)
   {
     MapList mapping = null;
-
+    SequenceI dsmapFrom = mapFrom.getDatasetSequence() == null ? mapFrom
+            : mapFrom.getDatasetSequence();
+    SequenceI dsmapTo = mapTo.getDatasetSequence() == null ? mapTo : mapTo
+            .getDatasetSequence();
     /*
-     * look for a reverse mapping, if found make its inverse
+     * look for a reverse mapping, if found make its inverse. 
+     * Note - we do this on dataset sequences only.
      */
-    if (mapTo.getDBRefs() != null)
+    if (dsmapTo.getDBRefs() != null)
     {
-      for (DBRefEntry dbref : mapTo.getDBRefs())
+      for (DBRefEntry dbref : dsmapTo.getDBRefs())
       {
         String name = dbref.getSource() + "|" + dbref.getAccessionId();
-        if (dbref.hasMap() && mapFrom.getName().startsWith(name))
+        if (dbref.hasMap() && dsmapFrom.getName().startsWith(name))
         {
           /*
            * looks like we've found a map from 'mapTo' to 'mapFrom'
            * - invert it to make the mapping the other way 
            */
           MapList reverse = dbref.getMap().getMap().getInverse();
-          xref.setMap(new Mapping(mapTo, reverse));
-          mappings.addMap(mapFrom, mapTo, reverse);
+          xref.setMap(new Mapping(dsmapTo, reverse));
+          mappings.addMap(mapFrom, dsmapTo, reverse);
           return true;
         }
       }
@@ -680,6 +907,22 @@ public class CrossRef
       return false;
     }
     xref.setMap(new Mapping(mapTo, mapping));
+
+    /*
+     * and add a reverse DbRef with the inverse mapping
+     */
+    if (mapFrom.getDatasetSequence() != null && false)
+    // && mapFrom.getDatasetSequence().getSourceDBRef() != null)
+    {
+      // possible need to search primary references... except, why doesn't xref
+      // == getSourceDBRef ??
+      // DBRefEntry dbref = new DBRefEntry(mapFrom.getDatasetSequence()
+      // .getSourceDBRef());
+      // dbref.setMap(new Mapping(mapFrom.getDatasetSequence(), mapping
+      // .getInverse()));
+      // mapTo.addDBRef(dbref);
+    }
+
     if (fromDna)
     {
       AlignmentUtils.computeProteinFeatures(mapFrom, mapTo, mapping);
@@ -703,11 +946,11 @@ public class CrossRef
    *          context was searching from Protein sequences
    * @param sequenceI
    * @param lrfs
-   * @param rseqs
+   * @param foundSeqs
    * @return true if matches were found.
    */
   private boolean searchDatasetXrefs(boolean fromDna, SequenceI sequenceI,
-          DBRefEntry[] lrfs, List<SequenceI> rseqs, AlignedCodonFrame cf)
+          DBRefEntry[] lrfs, List<SequenceI> foundSeqs, AlignedCodonFrame cf)
   {
     boolean found = false;
     if (lrfs == null)
@@ -720,7 +963,7 @@ public class CrossRef
       // add in wildcards
       xref.setVersion(null);
       xref.setMap(null);
-      found |= searchDataset(fromDna, sequenceI, xref, rseqs, cf, false);
+      found |= searchDataset(fromDna, sequenceI, xref, foundSeqs, cf, false);
     }
     return found;
   }
@@ -732,20 +975,29 @@ public class CrossRef
    * @param fromDna
    *          true if context was searching for refs *from* dna sequence, false
    *          if context was searching for refs *from* protein sequence
-   * @param sequenceI
+   * @param fromSeq
    *          a sequence to ignore (start point of search)
    * @param xrf
    *          a cross-reference to try to match
-   * @param rseqs
+   * @param foundSeqs
    *          result list to add to
-   * @param cf
+   * @param mappings
    *          a set of sequence mappings to add to
    * @param direct
-   *          - search all references or only subset
+   *          - indicates the type of relationship between returned sequences,
+   *          xrf, and sequenceI that is required.
+   *          <ul>
+   *          <li>direct implies xrf is a primary reference for sequenceI AND
+   *          the sequences to be located (eg a uniprot ID for a protein
+   *          sequence, and a uniprot ref on a transcript sequence).</li>
+   *          <li>indirect means xrf is a cross reference with respect to
+   *          sequenceI or all the returned sequences (eg a genomic reference
+   *          associated with a locus and one or more transcripts)</li>
+   *          </ul>
    * @return true if relationship found and sequence added.
    */
-  boolean searchDataset(boolean fromDna, SequenceI sequenceI,
-          DBRefEntry xrf, List<SequenceI> rseqs, AlignedCodonFrame cf,
+  boolean searchDataset(boolean fromDna, SequenceI fromSeq, DBRefEntry xrf,
+          List<SequenceI> foundSeqs, AlignedCodonFrame mappings,
           boolean direct)
   {
     boolean found = false;
@@ -768,9 +1020,13 @@ public class CrossRef
           if (nxt.getDatasetSequence() != null)
           {
             System.err
-                    .println("Implementation warning: getProducts passed a dataset alignment without dataset sequences in it!");
+                    .println("Implementation warning: CrossRef initialised with a dataset alignment with non-dataset sequences in it! ("
+                            + nxt.getDisplayId(true)
+                            + " has ds reference "
+                            + nxt.getDatasetSequence().getDisplayId(true)
+                            + ")");
           }
-          if (nxt == sequenceI || nxt == sequenceI.getDatasetSequence())
+          if (nxt == fromSeq || nxt == fromSeq.getDatasetSequence())
           {
             continue;
           }
@@ -779,8 +1035,7 @@ public class CrossRef
            * complementary type if !direct
            */
           {
-            boolean isDna = Comparison
-                    .isNucleotide(new SequenceI[] { nxt });
+            boolean isDna = !nxt.isProtein();
             if (direct ? (isDna != fromDna) : (isDna == fromDna))
             {
               // skip this sequence because it is wrong molecule type
@@ -791,56 +1046,54 @@ public class CrossRef
           // look for direct or indirect references in common
           DBRefEntry[] poss = nxt.getDBRefs();
           List<DBRefEntry> cands = null;
-          /*
-           * TODO does this make any sense?
-           * if 'direct', search the dbrefs for xrf
-           * else, filter the dbrefs by type and then search for xrf
-           * - the result is the same isn't it?
-           */
-          if (direct)
-          {
-            cands = DBRefUtils.searchRefs(poss, xrf);
-          }
-          else
-          {
-            poss = DBRefUtils.selectDbRefs(!fromDna, poss);
-            cands = DBRefUtils.searchRefs(poss, xrf);
-          }
+
+          // todo: indirect specifies we select either direct references to nxt
+          // that match xrf which is indirect to sequenceI, or indirect
+          // references to nxt that match xrf which is direct to sequenceI
+          cands = DBRefUtils.searchRefs(poss, xrf);
+          // else
+          // {
+          // poss = DBRefUtils.selectDbRefs(nxt.isProtein()!fromDna, poss);
+          // cands = DBRefUtils.searchRefs(poss, xrf);
+          // }
           if (!cands.isEmpty())
           {
-            if (!rseqs.contains(nxt))
+            if (foundSeqs.contains(nxt))
             {
-              found = true;
-              rseqs.add(nxt);
-              if (cf != null)
+              continue;
+            }
+            found = true;
+            foundSeqs.add(nxt);
+            if (mappings != null && !direct)
+            {
+              /*
+               * if the matched sequence has mapped dbrefs to
+               * protein product / cdna, add equivalent mappings to
+               * our source sequence
+               */
+              for (DBRefEntry candidate : cands)
               {
-                // don't search if we aren't given a codon map object
-                for (DBRefEntry candidate : cands)
+                Mapping mapping = candidate.getMap();
+                if (mapping != null)
                 {
-                  Mapping mapping = candidate.getMap();
-                  if (mapping != null)
+                  MapList map = mapping.getMap();
+                  if (mapping.getTo() != null
+                          && map.getFromRatio() != map.getToRatio())
                   {
-                    MapList map = mapping.getMap();
-                    if (mapping.getTo() != null
-                            && map.getFromRatio() != map.getToRatio())
+                    /*
+                     * add a mapping, as from dna to peptide sequence
+                     */
+                    if (map.getFromRatio() == 3)
                     {
-                      // get sense of map correct for adding to product
-                      // alignment.
-                      if (fromDna)
-                      {
-                        // map is from dna seq to a protein product
-                        cf.addMap(sequenceI, nxt, map);
-                      }
-                      else
-                      {
-                        // map should be from protein seq to its coding dna
-                        cf.addMap(nxt, sequenceI, map.getInverse());
-                      }
+                      mappings.addMap(nxt, fromSeq, map);
+                    }
+                    else
+                    {
+                      mappings.addMap(nxt, fromSeq, map.getInverse());
                     }
                   }
                 }
               }
-              // TODO: add mapping between sequences if necessary
             }
           }
         }