Repository: seung-lab/cloud-volume

Issue:
Handling of zeros by .get_roots() 
Passing lists of supervoxel IDs containing zeros to CloudVolume.get_roots() causes only the first zero in the list to be dropped, while subsequent zeros return a root ID of zero. I am not sure if this is intentional, but this could cause indexing headaches if it isn't noticed and handled afterwards. 

Discussion:
jefferis: Hi @william-silversmith just checking that we still get an array back? But you just dedup on the way to the server and then put everything back on the way back out to the user?
william-silversmith: Yea, there should be no change to the user interface. I'm screening for zeros, duplicates, and IDs that are already root IDs. I send a unique array to the server and then remap it back to the input array. 

This seems inefficient to me (why don't we return a dict) but I don't want to break backwards compatibility.
jefferis: > Yea, there should be no change to the user interface. I'm screening for zeros, duplicates, and IDs that are already root IDs. I send a unique array to the server and then remap it back to the input array.

Thanks for confirming. This is what I am already doing in my [own wrapper](https://github.com/natverse/fafbseg/blob/master/R/flywire-api.R#L148). I guess it will be useful for python people using cloudvolume directly. In my own experience this is actually the behaviour that the end user normally wants. For example you might have a dataframe with a set of xyz locations that you have mapped to supervoxels. You want to identify the rootid for each row and not worry about mapping a dictionary back onto the rows.
william-silversmith: I suppose that's true, but these days it's pretty easy to call `fastremap.remap`. I just never like providing a basic API that's intentionally inefficient so that people with more pressing needs are locked in. :(

Patch:
diff --git a/cloudvolume/frontends/graphene.py b/cloudvolume/frontends/graphene.py
index 0830d90..b1206ae 100644
--- a/cloudvolume/frontends/graphene.py
+++ b/cloudvolume/frontends/graphene.py
@@ -286,13 +286,23 @@ class CloudVolumeGraphene(CloudVolumePrecomputed):
         Layer 2+: Between chunk interconnections (skip connections possible)
     """
     segids = toiter(segids)
-    if isinstance(segids, np.ndarray):
-      segids = segids.tolist()
+    input_segids = np.array(segids, dtype=self.meta.dtype)
 
-    try:
-      segids.remove(0) # background segid
-    except ValueError:
-      pass
+    if input_segids.size == 0:
+      return np.array([], dtype=self.meta.dtype)
+
+    segids = fastremap.unique(input_segids)
+
+    base_remap = { 0: 0 }
+    # skip ids that are already root IDs
+    for segid in segids:
+      if self.meta.decode_layer_id(segid) == self.meta.n_layers:
+        base_remap[segid] = segid
+
+    segids = np.array(
+      [ segid for segid in segids if segid not in base_remap ], 
+      dtype=self.meta.dtype
+    )
 
     timestamp = to_unix_time(timestamp)
 
@@ -313,7 +323,10 @@ class CloudVolumeGraphene(CloudVolumePrecomputed):
         + ", ".join([ str(_) for _ in self.meta.supported_api_versions ])
       )
 
-    return np.array(roots, dtype=self.meta.dtype)
+    for segid, root_id in zip(segids, roots):
+      base_remap[segid] = root_id
+
+    return fastremap.remap(input_segids, base_remap)
 
   def get_chunk_mappings(self, chunk_id, timestamp=None):
     """
@@ -369,6 +382,9 @@ class CloudVolumeGraphene(CloudVolumePrecomputed):
     return chunk_mappings
 
   def _get_roots_v1(self, segids, timestamp, binary=False, stop_layer=None):
+    if len(segids) == 0:
+      return []
+
     args = {}
 
     headers = {}
@@ -412,6 +428,9 @@ class CloudVolumeGraphene(CloudVolumePrecomputed):
       return orjson.loads(response.content)['root_ids']
 
   def _get_roots_legacy(self, segids, timestamp):
+    if len(segids) == 0:
+      return []
+
     args = {}
     if timestamp is not None:
       args['timestamp'] = timestamp


Test patch:
diff --git a/test/test_graphene.py b/test/test_graphene.py
index 836f16c..ac403e5 100644
--- a/test/test_graphene.py
+++ b/test/test_graphene.py
@@ -331,8 +331,20 @@ def graphene_vol(cv_supervoxels,  requests_mock, monkeypatch, N=64):
     "data_dir": cv_supervoxels,
     "data_type": "uint64",
     "graph": {
-      "chunk_size": [64, 64, 64]
+      "chunk_size": [64, 64, 64],
+      "bounding_box": [2048, 2048, 512], 
+      "chunk_size": [256, 256, 512], 
+      "cv_mip": 0, 
+      "n_bits_for_layer_id": 8, 
+      "n_layers": 12, 
+      "spatial_bit_masks": {
+        '1': 10, '2': 10, '3': 9, 
+        '4': 8, '5': 7, '6': 6, 
+        '7': 5, '8': 4, '9': 3, 
+        '10': 2, '11': 1, '12': 1
+      }
     },
+    "app": { "supported_api_versions": [0, 1] },
     "mesh": "mesh_mip_2_err_40_sv16",
     "num_channels": 1,
     "scales": [
@@ -371,6 +383,18 @@ def test_gcv(graphene_vol):
   assert graphene_vol[0,0,0].shape == (1,1,1,1)
 
 
+def test_get_roots(graphene_vol):
+  roots = graphene_vol.get_roots([])
+  assert np.all(roots == [])
+
+  segids = [0, 0, 0, 0, 0]
+  roots = graphene_vol.get_roots(segids)
+  assert np.all(roots == segids)
+
+  segids = [0, 864691135462849854, 864691135462849854, 0]
+  roots = graphene_vol.get_roots(segids)
+  assert np.all(roots == segids)
+
 def faces_to_edges(faces, return_index=False):
   """
   Given a list of faces (n,3), return a list of edges (n*3,2)