EvMapping: broken comparison

[z2v]

EvMapping object from PyDSTool/ModelTools.py defines __cmp__ method. This method returns True or False. That contradicts to Python 2 demands (Python 2 data model - method must return integer, and 0 means 'eqaulity'). As a result, 'equality' and 'inequality' of EvMapping objects are reverted: when __cmp__ returns False, this means 'objects are equal' (int(False) == 0).

Replacing __cmp__ with rich comparison operators (a must for Python 3, see patch below) leads to 'RuntimeError' in _applyStateMap method of HybridModel object (Model.py, lines 2205-2207) both on Python 2 and Python 3. To get error, apply patch

diff --git a/PyDSTool/ModelTools.py b/PyDSTool/ModelTools.py
index b74102c..312809b 100755
--- a/PyDSTool/ModelTools.py
+++ b/PyDSTool/ModelTools.py
@@ -1536,7 +1536,7 @@ class EvMapping(object):
         self.makeCallFn()


-    def __cmp__(self, other):
+    def __eq__(self, other):
         try:
             return alltrue([self.assignDict==other.assignDict,
                             self.defString==other.defString,
@@ -1544,6 +1544,11 @@ class EvMapping(object):
         except AttributeError:
             return False

+    def __ne__(self, other):
+        return not self == other
+
+    __hash__ = None
+
     def makeCallFn(self):
         """Note that the function created alters xdict, pdict, idict, and estruct
         *in place*, and does not return any values.

and run 'examples/IF_delaynet_syn.py'.

So either 'equatlity' checking in EvMapping, or check in _applyStateMap (or both) shoud be adjusted.

[robclewley]

@z2v Thanks for the info. I don't have time to study this in detail right now, but if the changes are limited in scope, as they appear to be, can you make a suggestion on which to adjust? Off the top of my head, I don't know of a reason to care which way it is changed. Otherwise, I can study this in a couple of weeks' time.

[z2v]

@robclewley, we must replace __cmp__ with __eq__ and __ne__ for Python 3 support, anyway. So I suggest to remove check in _applyStateMap at all.

[robclewley]

@z2v OK, but which check are you referring to in HybridModel._applyStateMap? The only one I see is the check for simultaneous events, which is important to trap. Did I miss it?

[z2v]

@robclewley , I talk about this check:

        if num_maps > 1:
            # ensure that all the maps are the same for multiple
            # simultaneous events that point to the same generator
            # (the latter was already verified above)
            for mapix in range(1,num_maps):
                if epochStateMaps[0] != epochStateMaps[mapix]:
                    raise RuntimeError("PyDSTool does not yet allow "
                         "truly simultaneous events that do not point "
                         "to the same model with the same mapping")

After replacing __cmp__ in EvMapping, condition (!=) always equals to True. At least, for 'examples/IF_delaynet_syn.py` example.

[robclewley]

@z2v That's what I thought. This might need closer investigation, then, because that check is important. Without it, rare cases where there are simultaneous events will go unnoticed and could result in spurious numerical results. I don't understand how the simple switch from __cmp__ to __eq__ would break this.

[z2v]

@robclewley, ok. I'll try to describe, what I've found.

In current state, 'examples/IF_delaynet_syn.py' example passes on Python 2 and fails on Python 3 with RuntimeError due to referred check. The reason is: __cmp__ has been removed from Python 3, so this method in EvMapping ignored and objects are compared by id, so they are not equal.

On Python 2, __cmp__ in this concrete situation returns False (defString attributes of mappings differ, so alltrue return False - see here). Python 2 converts False to 0 (__cmp__ must return integer!), which means, that objects are equal, and referred check of mappings results in False, so RuntimeError is not raised.

[robclewley]

@z2v I understand so far about the current state. But you added the rich comparisons in the patch and said that still leads to a problem. Why do the rich comparison methods not ensure the desired comparison? In the usual ("everything OK") case, the new __ne__ will return False for the identically defined event mappings (they should have the same assignDict and defString -- are you saying they don't?) and this if statement should therefore not result in the error condition. Sorry, I still seem to be missing a step in the logic or what you are proposing!

[z2v]

@robclewley, if I replace __cmp__ with rich comparison, example starts to fail with RuntimeError on Python 2 too. Because compared EvMapping objects are really different (they do have diferrent defString attributes).

So logic is simple.

• If this concrete example ('IF_delaynet_syn.py') should pass, then either comparing, or check is wrong and should be fixed
• If check and comparison are correct, then example must fail and it's current behaviour on Python 2 should be fixed

[robclewley]

There is indeed something wrong with the logical principle of this check (beyond the technical issue of cmp vs eq implementation). Currently, later in the method, there's a warning from the previous hybrid segment if more than one event was found (provided verboselevel > 0) and that should take care of it for now. (In fact, an extra clause to make that an error for verboselevel > 1 would be good.) When this EvMapping comparison is reached, there may indeed be multiple EvMappings and they have not been correctly populated: they should only be compared if they are associated with activated events at this timestep. So, this test is spurious and can be commented out. Please leave a note in the code to address it later, preferably with a link to this discussion?