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

Submodel decorators need to do name-mangling #591

Closed jlstevens closed 9 years ago

jlstevens commented 9 years ago

Right now, decorating a method with a sheet decorator ignores the identity of the class where the decoration occurs. If the decorator tracks the class name, this issue could be resolved.

jlstevens commented 9 years ago

Unfortunately, solving this isn't as simple as I would have hoped.

It seems that decorators on class methods have no idea what class they occur in - all the decorator sees is a plain functions (not even an unbound method).

For now, the only way I can this work is to have a class method on Model called register. Then for each class that uses the decorators you do:

class EarlyVisionModel(VisualInputModel):
    Model.register('EarlyVisionModel')

class ModelGCAL(EarlyVisionModel):
    Model.register('ModelGCAL')

class ColorEarlyVisionModel(EarlyVisionModel):
    Model.register('ColorEarlyVisionModel')

class ModelGCALInheritingColor(ColorEarlyVisionModel):
     Model.register('ModelGCALInheritingColor')

Then it is possible to convey the class name to the decorators as they are applied. Finally, this could be used to ensure the exact right method is used by working against the class mro().

This isn't brilliant but I can't see a better way unless we abandon decorators entirely and go back to dictionaries of methods - which I thought was horrible!

Another way would be to name the class you are in every time you use a decorator but that would be very ugly and verbose. Hopefully, there is a better solution that doesn't involve inspecting stack frames or anything else too horrible!

jlstevens commented 9 years ago

Thanks to the wonders of StackOverflow, I've found a solution that is probably better:

def ModelClass(cls):
    "This is a class decorator for Model classes"
    for name, method in cls.__dict__.iteritems():
        if hasattr(method, "object_type"):
            # do something with the method and class
            print name, cls.__name__, getattr(method, 'object_type')
    return cls

def GeneratorSheet(view):
    "This represents one of our current method decorators"
    view.object_type = 'GeneratorSheet'
    return view

# Example 1
@ModelClass
class Model(object):
    @GeneratorSheet
    def Retina(self):
        print "Inside method of Model"

# Example 2
@ModelClass
class ModelDerived(object):
    @GeneratorSheet
    def Retina(self):
        print "Inside method of ModelDerived"

Output:

Retina Model GeneratorSheet
Retina ModelDerived GeneratorSheet

See the post I based this code off here. Note that the important point is that this can keep track of the method name, the type of decoration and the class it is in without needed to instantiate anything.

I think a single class decorator for Model classes could solve the issue.

jlstevens commented 9 years ago

This issue seemed to be important enough to prioritize. The fixes uses the class decorator method suggested about, implemented in commits 72c308a and ff5b3a49.

All tests are passing and I have checked you you can now import ModelGCALColor after ModelGCAL without affecting the use of ModelGCAL. The only issue right now is to do with the use of params and script params with lots of ''Setting non-parameter attribute'' warnings.

As an aside, ModelGCALColor should give a useless error message when not using the 'FoliageA' or 'FoliageB' datasets as anything else doesn't seem to work with colour properly.

jbednar commented 9 years ago

Both the register approach and adding the name to the decorator would work, but are ugly. The class decorator approach seems ok to me.

The University of Edinburgh is a charitable body, registered in Scotland, with registration number SC005336.

jlstevens commented 9 years ago

That's good because that is exactly what I implemented. :-)