ioam / topographica

A general-purpose neural simulator focusing on topographic maps.
topographica.org
BSD 3-Clause "New" or "Revised" License
53 stars 32 forks source link

Fix for issue #585 #589

Closed Tobias-Fischer closed 10 years ago

Tobias-Fischer commented 10 years ago

This PR provides a fix for the issue described in https://github.com/ioam/topographica/issues/585. The issue is related to the match conditions, and the problematic lines are the following:

for incoming_key, incoming_value in matchconditions.items():
          if incoming_key in src_sheet.properties and \
                 str(src_sheet.properties[incoming_key]) not in str(incoming_value):
              matches=False
              break

There are two issues with that. For easier description, see the following matchcondition:

{'level': 'LGN', 'polarity':properties['polarity'],
                 'opponent':properties['opponent'],
                 'surround':properties['surround']}

What we want to achieve is that this sheet connects to all sheets which have the following properties: a) level is LGN AND b) polarity is the same as the dest sheet AND c) opponent is the same as the dest sheet AND d) surround is the same as the dest sheet

Using the code above, there will always be a match, except one of the conditions is false. But when is it false? Only if the src sheet also has the key in its properties and the corresponding value to this is not contained in the property value of the dest sheet. Both very vague assumptions, which needed clarification. If the key is not in the properties of the src sheet, there should not be a match (here, it caused a color sheet to be matched with a SF sheet, because c) and d) were skipped and a) and b) matched). Furthermore, there should be an exact match between the property values of src and dest sheet. Historically, I was not able to make it an exact match, as I was using the old color model with separate retinas. The non-existing check whether the key is contained in the properties in the source sheet always was a bug, but never seemed to cause issues.

The same mistake was made in topo/submodel/specifications.py, where startswith was used rather than an exact match. There, it was only used for showing a summary of the model, but still it caused confusion in more complex models.

Finally, two separate matchconditions were merged for the color model.

Tobias-Fischer commented 10 years ago

Sorry, I made a small mistake. Thinking about it again (I wonder when I will be able to stop thinking about the project ;-) ), I figured that there might be an issue with the 'None' values. In particular, no gain control projections for a normal OR-only simulation were created anymore!! The fix for the fix: Change

elif incoming_key not in src_sheet.properties:

To

elif incoming_key not in src_sheet.properties and incoming_value is not None:

In line 441 of topo/submodel/init.py

To my mind even nicer, merge the two if statements to then look like:

        for incoming_key, incoming_value in matchconditions.items():
            if (incoming_key in src_sheet.properties and \
                    str(src_sheet.properties[incoming_key]) != str(incoming_value)) or \
               (incoming_key not in src_sheet.properties and incoming_value is not None):
                    matches=False
                    break
jbednar commented 10 years ago

D'oh! That was a nasty bug. I've committed your fix in 3374843. When I tried gcal.ty before the fix, I didn't see any problem with GC projections, but there weren't any projections from the retina to the LGN, so there was no response at all. Afterwards, response seems normal, and GC is operating as usual. We'll see how the tests do when they run today. Thanks for the fix!

jlstevens commented 10 years ago

This fix looks fine to me and the training tests are still passing.

Tobias: There were a couple of lines to do with changing randomness (which would break the tests). I've lost track of where they've gone - will there be a PR for that?