JAL-2225 fix bug in collating unique pdb files and their sequences

diff --git a/src/jalview/gui/StructureChooser.java b/src/jalview/gui/StructureChooser.java
index da10e3f..cf8c190 100644 (file)
--- a/src/jalview/gui/StructureChooser.java
+++ b/src/jalview/gui/StructureChooser.java
@@ -923,16 +923,10 @@ public class StructureChooser extends GStructureChooser
     }
     if (pdbEntriesToView.length > 1)
     {
-      ArrayList<SequenceI[]> seqsMap = new ArrayList<SequenceI[]>();
-      for (SequenceI seq : sequences)
-      {
-        seqsMap.add(new SequenceI[] { seq });
-      }
-      SequenceI[][] collatedSeqs = seqsMap.toArray(new SequenceI[0][0]);
       ssm.setProgressBar(null);
       ssm.setProgressBar(MessageManager.getString(
               "status.fetching_3d_structures_for_selected_entries"));
-      sViewer.viewStructures(pdbEntriesToView, collatedSeqs, alignPanel);
+      sViewer.viewStructures(pdbEntriesToView, sequences, alignPanel);
     }
     else
     {

diff --git a/src/jalview/gui/StructureViewer.java b/src/jalview/gui/StructureViewer.java
index e58b378..a1913c6 100644 (file)
--- a/src/jalview/gui/StructureViewer.java
+++ b/src/jalview/gui/StructureViewer.java
@@ -29,7 +29,11 @@ import jalview.structure.StructureSelectionManager;
 
 import java.awt.Rectangle;
 import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 
 /**
  * proxy for handling structure viewers.
@@ -76,120 +80,155 @@ public class StructureViewer
    * @return
    */
   public JalviewStructureDisplayI viewStructures(PDBEntry[] pdbs,
-          SequenceI[][] seqsForPdbs, AlignmentPanel ap)
+          SequenceI[] seqsForPdbs, AlignmentPanel ap)
   {
     JalviewStructureDisplayI viewer = onlyOnePdb(pdbs, seqsForPdbs, ap);
     if (viewer != null)
     {
+      /*
+       * user added structure to an existing viewer - all done
+       */
       return viewer;
     }
-    return viewStructures(getViewerType(), pdbs, seqsForPdbs, ap);
+
+    ViewerType viewerType = getViewerType();
+
+    // old way:
+    // PDBEntry[] pdbsForFile = getUniquePdbFiles(pdbs);
+
+    // new way:
+    Map<PDBEntry, SequenceI[]> seqsForPdb = getSequencesForPdbs(pdbs,
+            seqsForPdbs);
+    PDBEntry[] pdbsForFile = seqsForPdb.keySet().toArray(
+            new PDBEntry[seqsForPdb.size()]);
+    SequenceI[][] theSeqs = seqsForPdb.values().toArray(
+            new SequenceI[seqsForPdb.size()][]);
+    JalviewStructureDisplayI sview = null;
+    if (viewerType.equals(ViewerType.JMOL))
+    {
+      sview = new AppJmol(ap, pdbsForFile, theSeqs);
+      // ap.av.collateForPDB(pdbsForFile));
+    }
+    else if (viewerType.equals(ViewerType.CHIMERA))
+    {
+      sview = new ChimeraViewFrame(pdbsForFile, theSeqs, ap);
+      // ap.av.collateForPDB(pdbsForFile), ap);
+    }
+    else
+    {
+      Cache.log.error("Unknown structure viewer type "
+              + getViewerType().toString());
+    }
+    return sview;
   }
 
   /**
-   * A strictly temporary method pending JAL-1761 refactoring. Determines if all
-   * the passed PDB entries are the same (this is the case if selected sequences
-   * to view structure for are chains of the same structure). If so, calls the
-   * single-pdb version of viewStructures and returns the viewer, else returns
-   * null.
+   * Converts the list of selected PDB entries (possibly including duplicates
+   * for multiple chains), and corresponding sequences, into a map of sequences
+   * for each distinct PDB file. Returns null if either argument is null, or
+   * their lengths do not match.
    * 
    * @param pdbs
-   * @param seqsForPdbs
-   * @param ap
+   * @param seqs
    * @return
    */
-  private JalviewStructureDisplayI onlyOnePdb(PDBEntry[] pdbs,
-          SequenceI[][] seqsForPdbs, AlignmentPanel ap)
+  static Map<PDBEntry, SequenceI[]> getSequencesForPdbs(PDBEntry[] pdbs,
+          SequenceI[] seqs)
   {
-    List<SequenceI> seqs = new ArrayList<SequenceI>();
-    if (pdbs == null || pdbs.length == 0)
+    if (pdbs == null || seqs == null || pdbs.length != seqs.length)
     {
       return null;
     }
-    int i = 0;
-    String firstFile = pdbs[0].getFile();
-    for (PDBEntry pdb : pdbs)
+
+    /*
+     * we want only one 'representative' PDBEntry per distinct file name
+     * (there may be entries for distinct chains)
+     */
+    Map<String, PDBEntry> pdbsSeen = new HashMap<>();
+
+    /*
+     * LinkedHashMap preserves order of PDB entries (significant if they
+     * will get superimposed to the first structure)
+     */
+    Map<PDBEntry, List<SequenceI>> pdbSeqs = new LinkedHashMap<>();
+    for (int i = 0; i < pdbs.length; i++)
     {
+      PDBEntry pdb = pdbs[i];
+      SequenceI seq = seqs[i];
       String pdbFile = pdb.getFile();
-      if (pdbFile == null || !pdbFile.equals(firstFile))
+      if (!pdbsSeen.containsKey(pdbFile))
       {
-        return null;
+        pdbsSeen.put(pdbFile, pdb);
+        pdbSeqs.put(pdb, new ArrayList<SequenceI>());
       }
-      SequenceI[] pdbseqs = seqsForPdbs[i++];
-      if (pdbseqs != null)
+      else
       {
-        for (SequenceI sq : pdbseqs)
-        {
-          seqs.add(sq);
-        }
+        pdb = pdbsSeen.get(pdbFile);
+      }
+      List<SequenceI> seqsForPdb = pdbSeqs.get(pdb);
+      if (!seqsForPdb.contains(seq))
+      {
+        seqsForPdb.add(seq);
       }
     }
-    return viewStructures(pdbs[0], seqs.toArray(new SequenceI[seqs.size()]),
-            ap);
-  }
 
-  public JalviewStructureDisplayI viewStructures(PDBEntry pdb,
-          SequenceI[] seqsForPdb, AlignmentPanel ap)
-  {
-    return viewStructures(getViewerType(), pdb, seqsForPdb, ap);
-  }
-
-  protected JalviewStructureDisplayI viewStructures(ViewerType viewerType,
-          PDBEntry[] pdbs, SequenceI[][] seqsForPdbs, AlignmentPanel ap)
-  {
-    PDBEntry[] pdbsForFile = getUniquePdbFiles(pdbs);
-    JalviewStructureDisplayI sview = null;
-    if (viewerType.equals(ViewerType.JMOL))
+    /*
+     * convert to Map<PDBEntry, SequenceI[]>
+     */
+    Map<PDBEntry, SequenceI[]> result = new LinkedHashMap<>();
+    for (Entry<PDBEntry, List<SequenceI>> entry : pdbSeqs.entrySet())
     {
-      sview = new AppJmol(ap, pdbsForFile,
-              ap.av.collateForPDB(pdbsForFile));
+      List<SequenceI> theSeqs = entry.getValue();
+      result.put(entry.getKey(),
+              theSeqs.toArray(new SequenceI[theSeqs.size()]));
     }
-    else if (viewerType.equals(ViewerType.CHIMERA))
-    {
-      sview = new ChimeraViewFrame(pdbsForFile,
-              ap.av.collateForPDB(pdbsForFile), ap);
-    }
-    else
-    {
-      Cache.log.error("Unknown structure viewer type "
-              + getViewerType().toString());
-    }
-    return sview;
+
+    return result;
   }
 
   /**
-   * Convert the array of PDBEntry into an array with no filename repeated
+   * A strictly temporary method pending JAL-1761 refactoring. Determines if all
+   * the passed PDB entries are the same (this is the case if selected sequences
+   * to view structure for are chains of the same structure). If so, calls the
+   * single-pdb version of viewStructures and returns the viewer, else returns
+   * null.
    * 
    * @param pdbs
+   * @param seqsForPdbs
+   * @param ap
    * @return
    */
-  static PDBEntry[] getUniquePdbFiles(PDBEntry[] pdbs)
+  private JalviewStructureDisplayI onlyOnePdb(PDBEntry[] pdbs,
+          SequenceI[] seqsForPdbs, AlignmentPanel ap)
   {
-    if (pdbs == null)
+    List<SequenceI> seqs = new ArrayList<SequenceI>();
+    if (pdbs == null || pdbs.length == 0)
     {
       return null;
     }
-    List<PDBEntry> uniques = new ArrayList<PDBEntry>();
-    List<String> filesSeen = new ArrayList<String>();
-    for (PDBEntry entry : pdbs)
+    int i = 0;
+    String firstFile = pdbs[0].getFile();
+    for (PDBEntry pdb : pdbs)
     {
-      String file = entry.getFile();
-      if (file == null)
+      String pdbFile = pdb.getFile();
+      if (pdbFile == null || !pdbFile.equals(firstFile))
       {
-        uniques.add(entry);
+        return null;
       }
-      else if (!filesSeen.contains(file))
+      SequenceI pdbseq = seqsForPdbs[i++];
+      if (pdbseq != null)
       {
-        uniques.add(entry);
-        filesSeen.add(file);
+        seqs.add(pdbseq);
       }
     }
-    return uniques.toArray(new PDBEntry[uniques.size()]);
+    return viewStructures(pdbs[0], seqs.toArray(new SequenceI[seqs.size()]),
+            ap);
   }
 
-  protected JalviewStructureDisplayI viewStructures(ViewerType viewerType,
-          PDBEntry pdb, SequenceI[] seqsForPdb, AlignmentPanel ap)
+  public JalviewStructureDisplayI viewStructures(PDBEntry pdb,
+          SequenceI[] seqsForPdb, AlignmentPanel ap)
   {
+    ViewerType viewerType = getViewerType();
     JalviewStructureDisplayI sview = null;
     if (viewerType.equals(ViewerType.JMOL))
     {

diff --git a/test/jalview/gui/StructureViewerTest.java b/test/jalview/gui/StructureViewerTest.java
index c1c1d5c..335863b 100644 (file)
--- a/test/jalview/gui/StructureViewerTest.java
+++ b/test/jalview/gui/StructureViewerTest.java
@@ -1,10 +1,17 @@
 package jalview.gui;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertSame;
+import static org.testng.Assert.assertTrue;
 
 import jalview.datamodel.PDBEntry;
 import jalview.datamodel.PDBEntry.Type;
+import jalview.datamodel.Sequence;
+import jalview.datamodel.SequenceI;
+
+import java.util.Map;
 
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
@@ -20,9 +27,9 @@ public class StructureViewerTest
   }
 
   @Test(groups = "Functional")
-  public void testGetUniquePdbFiles()
+  public void testGetSequencesForPdbs()
   {
-    assertNull(StructureViewer.getUniquePdbFiles(null));
+    assertNull(StructureViewer.getSequencesForPdbs(null, null));
 
     PDBEntry pdbe1 = new PDBEntry("1A70", "A", Type.PDB, "path1");
     PDBEntry pdbe2 = new PDBEntry("3A6S", "A", Type.PDB, "path2");
@@ -30,13 +37,46 @@ public class StructureViewerTest
     PDBEntry pdbe4 = new PDBEntry("1GAQ", "A", Type.PDB, null);
     PDBEntry pdbe5 = new PDBEntry("3A6S", "B", Type.PDB, "path2");
     PDBEntry pdbe6 = new PDBEntry("1GAQ", "B", Type.PDB, null);
+    PDBEntry[] pdbs = new PDBEntry[] { pdbe1, pdbe2, pdbe3, pdbe4, pdbe5,
+        pdbe6 };
+
+    /*
+     * seq1 ... seq6 associated with pdbe1 ... pdbe6
+     */
+    SequenceI[] seqs = new SequenceI[pdbs.length];
+    for (int i = 0; i < seqs.length; i++)
+    {
+      seqs[i] = new Sequence("Seq" + i, "abc");
+    }
 
     /*
-     * pdbe2 and pdbe5 get removed as having a duplicate file path
+     * pdbe3/5/6 should get removed as having a duplicate file path
      */
-    PDBEntry[] uniques = StructureViewer.getUniquePdbFiles(new PDBEntry[] {
-        pdbe1, pdbe2, pdbe3, pdbe4, pdbe5, pdbe6 });
-    assertEquals(uniques,
- new PDBEntry[] { pdbe1, pdbe2, pdbe4, pdbe6 });
+    Map<PDBEntry, SequenceI[]> uniques = StructureViewer
+            .getSequencesForPdbs(pdbs, seqs);
+    assertTrue(uniques.containsKey(pdbe1));
+    assertTrue(uniques.containsKey(pdbe2));
+    assertFalse(uniques.containsKey(pdbe3));
+    assertTrue(uniques.containsKey(pdbe4));
+    assertFalse(uniques.containsKey(pdbe5));
+    assertFalse(uniques.containsKey(pdbe6));
+
+    // 1A70 associates with seq1 and seq3
+    SequenceI[] ss = uniques.get(pdbe1);
+    assertEquals(ss.length, 2);
+    assertSame(seqs[0], ss[0]);
+    assertSame(seqs[2], ss[1]);
+
+    // 3A6S has seq2 and seq5
+    ss = uniques.get(pdbe2);
+    assertEquals(ss.length, 2);
+    assertSame(seqs[1], ss[0]);
+    assertSame(seqs[4], ss[1]);
+
+    // 1GAQ has seq4 and seq6
+    ss = uniques.get(pdbe4);
+    assertEquals(ss.length, 2);
+    assertSame(seqs[3], ss[0]);
+    assertSame(seqs[5], ss[1]);
   }
 }