topcat: handle point clouds differently for counting

Point counting was not working properly, because a count was made for
each point SubCloud, and you could have one of those for each subset.

So introduce a new class TableCloud which contains all the points with
the same position coordinates, but aggregates those with different
subsets.  In this way each position in the point cloud is only
iterated over once.  This fixes errors in the displayed point count,
and also means that other operations that need to iterate over
displayed positions (e.g. blob inclusion) can work more efficiently.

topcat/src/docs/sun253.xml

@@ -18285,6 +18285,7 @@ introduced since the last version:
             report mean values (true/true+false proportion).</li>
         <li>Fix TST input handler so TST files with fewer than 3 columns
             can be read.</li>
+        <li>Fix point counting in layer plots.</li>
         </ul>
         </p></dd>
 </dl>

topcat/src/main/uk/ac/starlink/topcat/plot2/GuiDataSpec.java

@@ -17,6 +17,7 @@
 
 /**
  * DataSpec implementation used by TOPCAT classes.
+ * All DataSpecs in use in the TOPCAT application are instances of this class.
  *
  * @author   Mark Taylor
  * @since    13 Mar 2013
@@ -110,6 +111,29 @@ public TopcatModel getTopcatModel() {
         return tcModel_;
     }
 
+    /**
+     * Returns the row subset forming the row mask for this dataspec.
+     *
+     * @return  row subset
+     */
+    public RowSubset getRowSubset() {
+        return subset_;
+    }
+
+    /**
+     * Returns the number of rows associated with this dataspec if it
+     * can be determined quickly.  If it would require a count, return -1.
+     *
+     * @return   row count or -1
+     */
+    public long getKnownRowCount() {
+        Map subsetCounts = tcModel_.getSubsetCounts();
+        Object countObj = subsetCounts.get( subset_ );
+        return countObj instanceof Number
+             ? ((Number) countObj).longValue()
+             : -1L;
+    }
+
     /**
      * Returns the number of rows associated with this data spec.
      * In most cases this will execute quickly, but if necessary a count
@@ -125,15 +149,13 @@ public long getRowCount() {
 
         /* If the row count for the relevant subset is already known,
          * use that. */
-        Map subsetCounts = tcModel_.getSubsetCounts();
-        Object countObj = subsetCounts.get( subset_ );
-        if ( countObj instanceof Number ) {
-            return ((Number) countObj).longValue();
+        long knownCount = getKnownRowCount();
+        if ( knownCount >= 0 ) {
+            return knownCount;
         }
 
         /* If not, count it now. */
         else {
-            assert countObj == null;
             long nrow = tcModel_.getDataModel().getRowCount();
             long count = 0;
             for ( long ir = 0; ir < nrow; ir++ ) {
@@ -143,7 +165,7 @@ public long getRowCount() {
             }
 
             /* Having got the result, save it for later. */
-            subsetCounts.put( subset_, new Long( count ) );
+            tcModel_.getSubsetCounts().put( subset_, new Long( count ) );
             return count;
         }
     }

topcat/src/main/uk/ac/starlink/topcat/plot2/GuiNavigationListener.java

@@ -61,6 +61,8 @@ public void run() {
     public Iterable<double[]> createDataPosIterable() {
 
         /* Handles progress reporting and thread interruption. */
-        return plotPanel_.createGuiPointCloud().createDataPosIterable();
+        GuiPointCloud pointCloud = plotPanel_.createGuiPointCloud();
+        return pointCloud
+              .createDataPosIterable( pointCloud.createGuiDataStore() );
     }
 }

topcat/src/main/uk/ac/starlink/topcat/plot2/GuiPointCloud.java

@@ -1,45 +1,56 @@
 package uk.ac.starlink.topcat.plot2;
 
+import java.awt.Point;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import java.util.Set;
 import javax.swing.BoundedRangeModel;
+import uk.ac.starlink.ttools.plot2.DataGeom;
+import uk.ac.starlink.ttools.plot2.PlotUtil;
 import uk.ac.starlink.ttools.plot2.PointCloud;
 import uk.ac.starlink.ttools.plot2.SubCloud;
 import uk.ac.starlink.ttools.plot2.data.DataStore;
+import uk.ac.starlink.ttools.plot2.data.TupleSequence;
 
 /**
- * Collects a set of SubClouds together to provide a description of a
+ * Collects a set of TableClouds together to provide a description of a
  * collection of positions in a plot.
  *
  * @author   Mark Taylor
  * @since    24 Jan 2014
  */
 public class GuiPointCloud {
 
-    private final SubCloud[] subClouds_;
+    private final TableCloud[] tclouds_;
+    private final int ndim_;
     private final DataStore baseDataStore_;
     private final BoundedRangeModel progModel_;
     private final long nrow_;
 
     /**
      * Constructor.
      *
-     * @param  subClouds   per-layer position collections
+     * @param  tclouds   per-table position collections
      * @param  baseDataStore  data store supplying the position data
      * @param  progModel   progress bar model; if non-null, iteration over
      *                     the points will update it
      */
-    public GuiPointCloud( SubCloud[] subClouds, DataStore baseDataStore,
+    public GuiPointCloud( TableCloud[] tclouds, DataStore baseDataStore,
                           BoundedRangeModel progModel ) {
-        subClouds_ = subClouds;
+        tclouds_ = tclouds;
+        ndim_ = tclouds.length > 0
+              ? tclouds[ 0 ].getDataGeom().getDataDimCount()
+              : 0;
         baseDataStore_ = baseDataStore;
         progModel_ = progModel;
 
         /* If we will be displaying progress, it is necessary to find out
-         * how many points there are in the data set. */
+         * how many points will be read to iterate over the data set. */
         if ( progModel != null ) {
             long nr = 0;
-            for ( int ic = 0; ic < subClouds.length; ic++ ) {
-                nr += ((GuiDataSpec) subClouds[ ic ].getDataSpec())
-                     .getRowCount();
+            for ( int ic = 0; ic < tclouds.length; ic++ ) {
+                nr += tclouds[ ic ].getReadRowCount();
             }
             nrow_ = nr;
         }
@@ -49,12 +60,12 @@ public GuiPointCloud( SubCloud[] subClouds, DataStore baseDataStore,
     }
 
     /**
-     * Returns the subclouds aggregated by this point cloud.
+     * Returns the TableClouds aggregated by this point cloud.
      *
-     * @return  subcloud array
+     * @return  table cloud array
      */
-    public SubCloud[] getSubClouds() {
-        return subClouds_;
+    public TableCloud[] getTableClouds() {
+        return tclouds_;
     }
 
     /**
@@ -71,12 +82,87 @@ public GuiDataStore createGuiDataStore() {
 
     /**
      * Returns an iterable over the point cloud.
-     * This uses the GUI data store.
      *
+     * @param  dataStore  data store
      * @return   iterable over data positions
      */
-    public Iterable<double[]> createDataPosIterable() {
-        return new PointCloud( subClouds_ )
-              .createDataPosIterable( createGuiDataStore() );
+    public Iterable<double[]>
+            createDataPosIterable( final DataStore dataStore ) {
+        return new Iterable<double[]>() {
+            public Iterator<double[]> iterator() {
+                return new DataPosIterator( dataStore );
+            }
+        };
+    }
+
+    /**
+     * Iterator over data positions in this cloud.
+     *
+     * <p>This implementation is mostly copied from PointCloud, it would
+     * be quite fiddly to subclass from the same code.
+     */
+    private class DataPosIterator implements Iterator<double[]> {
+        private final DataStore dataStore_;
+        private final Iterator<TableCloud> cloudIt_;
+        private final double[] dpos_;
+        private final double[] dpos1_;
+        private final Point gp_;
+        private DataGeom geom_;
+        private int iPosCoord_;
+        private TupleSequence tseq_;
+        private boolean hasNext_;
+
+        /**
+         * Constructor.
+         *
+         * @param  dataStore  data storage object
+         */
+        DataPosIterator( DataStore dataStore ) {
+            dataStore_ = dataStore;
+            cloudIt_ = Arrays.asList( tclouds_ ).iterator();
+            dpos_ = new double[ ndim_ ];
+            dpos1_ = new double[ ndim_ ];
+            gp_ = new Point();
+            tseq_ = PlotUtil.EMPTY_TUPLE_SEQUENCE;
+            hasNext_ = advance();
+        }
+
+        public boolean hasNext() {
+            return hasNext_;
+        }
+
+        public double[] next() {
+            if ( hasNext_ ) {
+                System.arraycopy( dpos_, 0, dpos1_, 0, ndim_ );
+                hasNext_ = advance();
+                return dpos1_;
+            }
+            else {
+                throw new NoSuchElementException();
+            }
+        }
+
+        public void remove() {
+            throw new UnsupportedOperationException();
+        }
+
+        /**
+         * Does work for the next iteration.
+         */
+        private boolean advance() {
+            while ( tseq_.next() ) {
+                if ( geom_.readDataPos( tseq_, iPosCoord_, dpos_ ) ) {
+                    return true;
+                }
+            }
+            while ( cloudIt_.hasNext() ) {
+                TableCloud tcloud = cloudIt_.next();
+                geom_ = tcloud.getDataGeom();
+                iPosCoord_ = tcloud.getPosCoordIndex();
+                tseq_ = tcloud.createTupleSequence( dataStore_ );
+                return advance();
+            }
+            return false;
+        }
     }
 }

topcat/src/main/uk/ac/starlink/topcat/plot2/PlotPanel.java

@@ -354,12 +354,14 @@ public PlotLayer[] getPlotLayers() {
     }
 
     /**
-     * Returns a list of point clouds giving the positions currently plotted.
+     * Returns a list of point clouds by subset giving the positions
+     * currently plotted.
      * The clouds are deduplicated, so that point sets from the same
      * table which are plotted more than once (as part of multiple layers)
-     * are not reported multiple times.
+     * are not reported multiple times.  However, the same point may
+     * appear in multiple subclouds, if it appears in more than one subset.
      *
-     * @return   distinct point clouds currently plotted
+     * @return  distinct point clouds by subset
      */
     public SubCloud[] getSubClouds() {
         return SubCloud.createSubClouds( workings_.layers_, true );
@@ -386,9 +388,11 @@ public DataStore getDataStore() {
      * @return  positions in most recent plot
      */
     public GuiPointCloud createGuiPointCloud() {
-        return new GuiPointCloud( getSubClouds(), getDataStore(),
-                                  showProgressModel_.isSelected() ? progModel_
-                                                                  : null );
+        return
+            new GuiPointCloud( TableCloud.createTableClouds( getSubClouds() ),
+                               getDataStore(),
+                               showProgressModel_.isSelected() ? progModel_
+                                                               : null );
     }
 
     /**
@@ -486,7 +490,7 @@ private PlotJob<P,A> createPlotJob() {
          * than at plot job creation, since creating a plot job does
          * not entail that it will ever be plotted, but it's likely
          * that the effect will be the same. */
-        SubCloud[] subClouds = getSubClouds();
+        SubCloud[] subClouds = SubCloud.createSubClouds( layers, true );
         highlightMap_.keySet().retainAll( Arrays.asList( subClouds ) );
         double[][] highlights = highlightMap_.values()
                                              .toArray( new double[ 0 ][] );