EvMapping: broken comparison about pydstool HOT 9 OPEN

@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.

from pydstool.

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

from pydstool.

@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?

from pydstool.

@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.

from pydstool.

@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.

from pydstool.

@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.

from pydstool.

@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!

from pydstool.

@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

from pydstool.

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?

from pydstool.