Change std::map pybindings to follow Python 3 dict semantics (#3576)

* Accept iterable keys in fromkeys

* make keys/values/items iterators

* WIP: minimal views for maps

* add mapping prop

* remove obsolute key/value/item lists

* scope tuple type under dict

keep them namespaces clean

* update dict interface test

* bump stubgen image

* add spdx tags

* dump stubgen image

to work around 7da09e7aac87e865f050da8d5893812f85016915

* regenerate stubs

* remove obsolete i3_map_extras

* ensure ignore comments are on the same line as signature

* Chase down old std map use in libs

Most of these were:
- implicit use of old pair iterator
- explicit use of iteritems() and friends

A whole bunch of copy-paste in iceprod modules was factored out into WriteI3SummaryWithUsage.

* Chase map API churn through scripts

* Remove bindings for std::map::value_type

This are no longer exposed to Python anywhere

* return values by reference in values()/items()

* remove methods not in MutableMapping

* remove obsolete __item_type__

* correct stubgen image sha

* add __len__ to views

* add set equality for views

* Chase map API churn through tests

* stubs: generate for 66c73d6adca438fcb06e0ffc7e5213d48e055fcc

Co-authored-by: Jakob van Santen <jvansanten@gmail.com>

* chase api churn through an entire copy-pasted file

* more test churn

* more test churn

* implicit NoProxy for map<K, shared_ptr<V>>

* more test churn

* more test churn

* more test churn

* more test churn

* Make map views dynamic

Store accessors that return iterators rather than iterators themselves,
so that views can be used after maps are modified

* clarify approach to set equality

* more test churn

* more test churn

* more test churn

* Actually remove commented-out code

Co-authored-by: Kevin Meagher <11620178+kjmeagher@users.noreply.github.com>

* stubs: generate for 637626e606f091acb2b526349b35ac3bfda24850

Co-authored-by: Jakob van Santen <jvansanten@gmail.com>

* Chase down more copypasta

* shiny for mypy post-merge

* fix up dict interface failures caught in tests

* chase down all remaining instances of has_key()

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Kevin Meagher <11620178+kjmeagher@users.noreply.github.com>

dataclasses/resources/test/std_map_indexing_suite_dict_interface_test.py

@@ -12,6 +12,7 @@
 # JvS 2009-03-27
 
 import unittest, sys
+from typing import Iterator
 
 try:
     sorted([3,2,1])
@@ -23,6 +24,8 @@ def sorted(lst):
         cpy.sort()
         return cpy
 
+from icecube import icetray,dataclasses
+
 class I3MapDictInterfaceTest(unittest.TestCase):
     """Kick the tires on the dict interface.
 
@@ -36,8 +39,7 @@ class I3MapDictInterfaceTest(unittest.TestCase):
     # fromkeys(k,None) (ditto)
     # update (kwargs as keys can only work with string maps)
     def setUp(self):
-        from icecube import icetray,dataclasses
-        self.mapClass = dataclasses.TestMapStringString
+        self.mapClass = icetray.map_string_pyobject
         self.dict = {'foo':'bar', 'bom':'baz', 'steam':'locomotive'}
         self.map = self.mapClass()
         for k in self.dict.keys(): self.map[k] = self.dict[k]
@@ -90,7 +92,8 @@ def test___hash__(self):
     def test___init__(self):
         """map accepts the same constructors as dict"""
         newMap = self.mapClass()
-        self.assertEqual(newMap.items(), [])
+        items = newMap.items()
+        self.assertEqual(list(items), [])
         newMap = self.mapClass(self.dict)
         self.assertEqual(sorted(list(newMap.items())), sorted(list(self.dict.items())))
         newMap = self.mapClass(list(self.dict.items()))
@@ -99,11 +102,7 @@ def test___init__(self):
         pass
     def test___iter__(self):
         """dict.__iter__() is equivalent to map.__iter__()"""
-        # dict iterates over keys, but the map returns
-        # wrapped pairs (i.e. items)
-        fromMap =  [iterate[0] for iterate in self.map]
-        fromDict = [iterate for iterate in self.dict]
-        self.assertEqual(sorted(fromMap), sorted(fromDict))
+        self.assertEqual(list(self.map), sorted(self.dict))
         pass
     def test___le__(self):
         """What does </> even mean for a dict? Skip."""
@@ -151,33 +150,6 @@ def test_clear(self):
         self.assertEqual([k for k,v in self.dict], [])
         self.assertEqual([k for k,v in self.map], [])
         pass
-    def test_copy(self):
-        """dict.copy() is equivalent to map.copy()"""
-        newMap = self.map.copy()
-        newDict = self.map.copy()
-        self.assertEqual(self.map.items(), newMap.items())
-        self.assertEqual(sorted(self.dict.items()), sorted(newDict.items()))
-        # FIXME: there seems to be no good way to test whether the copy was shallow
-        # confirm that the copy was shallow
-        # for dict_key,copied_dict_key in zip(sorted(self.dict.keys()),sorted(newDict.keys())):
-        #     self.assertEquals(id(dict_key),id(copied_dict_key))
-        #     self.assertEquals(id(self.dict[dict_key]),id(newDict[copied_dict_key]))
-        # for map_key,copied_map_key in zip(sorted(self.map.keys()),sorted(newMap.keys())):
-        #     self.assertEquals(id(map_key),id(copied_map_key))
-        #     self.assertEquals(id(self.map[map_key]),id(newMap[copied_map_key]))
-
-        pass
-    def test_fromkeys(self):
-        """dict.fromkeys() is equivalent to map.fromkeys()"""
-        v = list(self.dict.values())[0]
-        newDict = self.dict.fromkeys(self.dict.keys(),v)
-        newMap = self.map.fromkeys(self.map.keys(),v)
-        self.assertEqual(sorted(self.dict.keys()), sorted(newDict.keys()))
-        self.assertEqual(sorted(self.map.keys()), sorted(newMap.keys()))
-        for k in self.dict.keys():
-            self.assertEqual(newDict[k], v)
-            self.assertEqual(newMap[k], v)
-        pass
     def test_get(self):
         """dict.get() is equivalent to map.get()"""
         default = 42
@@ -200,7 +172,34 @@ def test_keys(self):
         dictItems = sorted(self.dict.keys())
         mapItems = sorted(self.map.keys())
         self.assertEqual(dictItems, mapItems)
-        pass
+
+        self.assertEqual(len(self.dict.keys()), len(self.map.keys()))
+
+        self.assertEqual(self.map.keys(), self.dict.keys())
+        self.assertNotEqual(self.dict.keys(), 1)
+        self.assertNotEqual(self.map.keys(), 1)
+
+        self.assertNotEqual(
+            self.map.keys(),
+            [list(self.map.values())[0]]*len(self.map),
+            "should compare nonequal, even if same length and all items contained"
+        )
+
+        mkv = self.map.keys()
+        # __contains__ should not exhaust the view like it would for an iterator
+        for _ in range(2):
+            for k in reversed(self.dict.keys()):
+                assert k in mkv
+
+        # our views aren't reversible
+        dkv = self.dict.keys()
+        with self.assertRaises(TypeError):
+            for k in reversed(self.map.keys()):
+                assert k in dkv
+
+        if sys.version_info >= (3,10):
+            self.assertEqual(dict(mkv.mapping), dkv.mapping)
+
     def test_pop(self):
         """dict.pop() is equivalent to map.pop()"""
         default = 42
@@ -240,8 +239,30 @@ def test_update(self):
     def test_values(self):
         """dict.values() is equivalent to map.values()"""
         self.assertEqual(sorted(self.map.values()), sorted(self.dict.values()))
-        pass
+        # values views are never equal to anything
+        self.assertNotEqual(self.map.values(), self.dict.values())
+        self.assertNotEqual(self.dict.values(), self.map.values())
+        self.assertNotEqual(self.dict.values(), self.dict.values())
+
+    def test_view_references(self):
+        m = dataclasses.I3MapIntVectorInt({0: []})
+        for v in m.values():
+            v.append(1)
+        self.assertListEqual([list(v) for v in m.values()], [[1]])
+        for k, v in m.items():
+            v.append(2)
+        self.assertEqual([list(v) for v in m.values()], [[1, 2]])
 
+    def test_view_dynamism(self):
+        m = icetray.map_int_int({i: i for i in range(5,10)})
+        # begin() currently points to 5
+        keys = m.keys()
+        self.assertEqual(len(list(keys)), 5)
+        m.update({i: i for i in range(2)})
+        # invalidate the result of the previous begin()
+        del m[5]
+        # nothing blows up, as iteration invokes begin() again 
+        self.assertEqual(len(list(keys)), 6)
 
 if __name__ == '__main__':
     unittest.main()

dataclasses/resources/test/std_map_indexing_suite_test.py

@@ -45,92 +45,25 @@ def testKeys(self):
     def testKeyIterables(self):
         """Various equivalent ways of getting the keys"""
         self.assertEqual(list(self.map.keys()), [ key           for key       in self.map.keys() ])  # noqa: SIM118
-        self.assertEqual(list(self.map.keys()), [ key           for key,value in self.map            ])
-        self.assertEqual(list(self.map.keys()), [ entry.key()   for entry     in self.map            ])
-        self.assertEqual(list(self.map.keys()), [ entry.first() for entry     in self.map            ])
+        self.assertEqual(list(self.map.keys()), [ key           for key,value in self.map.items()    ])
     def testValueIterables(self):
         """Various equivalent ways of getting the values"""
         self.assertEqual(list(self.map.values()), [ value          for value     in self.map.values() ])
-        self.assertEqual(list(self.map.values()), [ value          for key,value in self.map              ])
-        self.assertEqual(list(self.map.values()), [ entry.data()   for entry     in self.map              ])
-        self.assertEqual(list(self.map.values()), [ entry.second() for entry     in self.map              ])
+        self.assertEqual(list(self.map.values()), [ value          for key,value in self.map.items()  ])
     def testIterators(self):
         """Iterators work for keys, values, and (k,v) tuples"""
         self.assertEqual( type( next(iter(self.map.items()))  ),tuple)
         self.assertEqual( type( next(iter(self.map.keys()))   ), type( list(self.sourceDict.keys())[0]   ))
         self.assertEqual( type( next(iter(self.map.values())) ), type( list(self.sourceDict.values())[0] ))
-    def testHasKey(self):
-        """map.has_key() is equivalent to 'key in map'"""
+    def testContains(self):
+        """key in map"""
         key = list(self.sourceDict.keys())[0]
         self.assertEqual(key in self.map,True)
-        self.assertEqual(key in self.map, key in self.map)
     def testCopyKeys(self):
         """can produce a new, equivalent dict from sourceDict via dict.update(map)"""
         d = dict()
         d.update(self.map)
         self.assertEqual(self.sourceDict, d)
-    def testPairUnpacking(self):
-        """the wrapped std::pair can be unpacked automatically"""
-        for entry in self.map:
-            key,value = entry
-            self.assertEqual(key, entry.key())
-            self.assertEqual(key, entry.first())
-            self.assertEqual(value, entry.data())
-            self.assertEqual(value, entry.second())
-    def testPairIndexing(self):
-        """elements of the wrapped std::pair can be accessed by index"""
-        for entry in self.map:
-            self.assertEqual(entry[0], entry.key())
-            self.assertEqual(entry[-1], entry.data())
-            self.assertEqual(entry[-2], entry.key())
-            self.assertEqual(entry[1], entry.data())
-            self.assertRaises(IndexError, entry.__getitem__, -3)
-            self.assertRaises(IndexError, entry.__getitem__, 2)
-            self.assertEqual([entry.key(),entry.data()], list(entry))
-            self.assertEqual(len(entry), 2)
-
-class IterRunner:
-    def setUp(self):
-        self.map = dataclasses.I3MapStringDouble()
-        for i in range(10000):
-            v = i/1.0e4
-            k = str(hash(v))
-            self.map[k] = v
-    def iterClassic(self):
-        return [(pair.key(),pair.data()) for pair in self.map]
-    def iterClassicUnpack(self):
-        return [(key,value) for key,value in self.map]
-    def getItems(self):
-        return list(self.map.items())
-    def getItemsUnpack(self):
-        return [(key,value) for key,value in self.map.items()]
-    def iterItems(self):
-        return [item for item in self.map.items()]
-    def iterItemsUnpack(self):
-        return [(key,value) for key,value in self.map.items()]
-
-class I3MapStringDoublePerformanceTest(unittest.TestCase):
-    """Run some quick benchmarks"""
-    def setUp(self):
-        self.map = dataclasses.I3MapStringDouble()
-        for i in range(5000):
-            v = i/5.0e3
-            k = str(hash(v))
-            self.map[k] = v
-    def testPerformance(self):
-        this_script = os.path.basename(os.path.splitext(__file__)[0])
-        setup = 'from %s import IterRunner; case = IterRunner(); case.setUp()' % this_script
-        def runCase(meth,desc,num=100):
-            print('%3.dx %s:' % (num,desc))
-            results = timeit.Timer(stmt='case.%s()'%meth,setup=setup).timeit(num)
-            print('===> %.3f s' % results)
-        print('')
-        print('Performance improvements (and some regressions) with the new map interfaces:')
-        runCase('iterClassic','[(pair.key(),pair.data()) for pair in i3map]\t(the old way)')
-        runCase('iterClassicUnpack','[(key,value) for key,value in i3map]\t\t(with implicit unpacking)')
-        runCase('iterItemsUnpack','[(key,value) for key,value in i3map.iteritems()]\t(item iterator with implicit unpacking)')
-        runCase('iterItems','[item for item in i3map.iteritems()]\t\t(using iterators + list comprehensions)')
-        runCase('getItems','i3map.items()\t\t\t\t\t(internal iterator only)')
 
 class I3MapUnsignedUnsignedTest(I3MapStringDoubleTest):
     def setUp(self):

dataclasses/resources/test/test_I3AntennaGeo.py

@@ -47,7 +47,7 @@ def test_Serialize(self):
             newI3File.close()
 
         theMap = testFrame[frameName]
-        readIn = theMap[theMap.keys()[0]]
+        readIn = theMap[next(iter(theMap.keys()))]
 
         self.assertEqual(readIn, antgeo)
 

dataclasses/resources/test/test_I3IceActGeo.py

@@ -65,7 +65,7 @@ def test_Serialize(self):
             newI3File.close()
 
         theMap = testFrame[frameName]
-        readIn = theMap[theMap.keys()[0]]
+        readIn = theMap[next(iter(theMap.keys()))]
 
         self.assertEqual(readIn, iceactGeo)
 

dataclasses/resources/test/test_I3RecoPulseSeriesMapMask_pybindings.py

@@ -49,30 +49,34 @@ def testApply(self):
         mask = self.frame['Mask1']
         pulses = mask.apply(self.frame)
         self.assertEqual(len(pulses), 2)
-        self.assertEqual(len(pulses.values()[0]), 3)
-        self.assertEqual(len(pulses.values()[1]), 2)
+        values = list(pulses.values())
+        self.assertEqual(len(values[0]), 3)
+        self.assertEqual(len(values[1]), 2)
 
         mask = self.frame['Mask2']
         pulses = mask.apply(self.frame)
+        values = list(pulses.values())
         self.assertEqual(len(pulses), 2)
-        self.assertEqual(len(pulses.values()[0]), 2)
-        self.assertEqual(len(pulses.values()[1]), 3)
+        self.assertEqual(len(values[0]), 2)
+        self.assertEqual(len(values[1]), 3)
 
     def testCombine(self):
         mask1 = self.frame['Mask1']
         mask2 = self.frame['Mask2']
 
         combined = mask1 & mask2
         pulses = combined.apply(self.frame)
+        values = list(pulses.values())
         self.assertEqual(len(pulses), 2)
-        self.assertEqual(len(pulses.values()[0]), 2)
-        self.assertEqual(len(pulses.values()[1]), 2)
+        self.assertEqual(len(values[0]), 2)
+        self.assertEqual(len(values[1]), 2)
 
         combined = mask1 | mask2
         pulses = combined.apply(self.frame)
         self.assertEqual(len(pulses), 2)
-        self.assertEqual(len(pulses.values()[0]), 3)
-        self.assertEqual(len(pulses.values()[1]), 3)
+        values = list(pulses.values())
+        self.assertEqual(len(values[0]), 3)
+        self.assertEqual(len(values[1]), 3)
 
     def testQuery(self):
         mask1 = self.frame['Mask1']

dataclasses/resources/test/test_I3ScintGeo.py

@@ -59,7 +59,7 @@ def test_Serialize(self):
             newI3File.close()
 
         theMap = testFrame[frameName]
-        readIn = theMap[theMap.keys()[0]]
+        readIn = theMap[next(iter(theMap.keys()))]
 
         self.assertEqual(readIn, scintgeo)
 

dataclasses/resources/test/workout_pybindings.py

@@ -377,7 +377,7 @@ def testI3RecoPulse(self):
         rpsm = dataclasses.I3RecoPulseSeriesMap()
         rpsm[icetray.OMKey(1, 1)] = rps
 
-        for key, pseries in rpsm:
+        for key, pseries in rpsm.items():
             logging.log_debug("key: %s" % key)
             for pulse in pseries:
                 logging.log_debug("pulse: %s" % pulse)

icetray/private/pybindings/std_cont_pod/std_cont_pod.h

@@ -86,27 +86,4 @@ void reg(const char* name)
   from_python_sequence<vector<T>, variable_capacity_policy>();
 }
 
-template <typename Map>
-class i3_map_extras : public def_visitor<i3_map_extras<Map> >
-{
-  static list keys(Map const&  x)
-  {
-    list t;
-    for(typename Map::const_iterator it = x.begin(); it != x.end(); it++)
-      t.append(it->first);
-    return t;
-  }
-
-  template <typename Class>
-  void visit(Class& cl) const
-  {
-    // std::cout << __PRETTY_FUNCTION__ << "\n";
-    cl
-      .def("keys", &keys)
-      ;
-  }
-
-  friend class boost::python::def_visitor_access;
-};
-
 #endif

icetray/private/pybindings/std_cont_pod/std_cont_pod_map_int_int.cxx

@@ -37,7 +37,6 @@ void register_std_cont_pod_map_int_int()
 {
   class_<std::map<int, int> >("map_int_int")
     .def(std_map_indexing_suite<std::map<int, int> >())
-    .def(i3_map_extras<std::map<int, int> >())
     ;
 
 }