Tests for sparse implementation of topographica

[Tasignotas]

Implementation of #596

I have made some progress on writing the tests. However, the test fails, because the output after the first iteration doesn't match with the expected output. Is there something obviously wrong with my code?

[jlstevens]

Mostly looks good but I'm not sure you can just override projection.CFProjection in property_setup like that (which may explain the test failure). Instead, I would expect you to choose some specific projections to switch to sparse by overriding the appropriate method in SparseGCAL. For instance, it should be something like:

    @Model.SparseCFProjection
    def V1_afferent(self, src_properties, dest_properties):
        return super(SparseGCAL, self).V1_afferent(src_properties, dest_properties)

It should be very similar for all the projections you may want to make sparse (i.e. afferents to V1 and lateral inhibitory projections). Making sure you have registered the sparse projection with Model by executing Model.register_decorator(SparseCFProjection) beforehand.

In addition, I would start by switching just one projection to sparse in this way to see if the test passes before trying to change several at once. Let's see if that works!

[Tasignotas]

Thanks for your suggestion. It looks like I implemented overriding the projection the way you suggested. However, segmentation fault occurs during the test. I can't figure out why this happens - it seems like there could be a mistake somewhere in the Cython wrapper for eigen3. Any thoughts?

[jlstevens]

I think I've seen segfaults in sparse projections if you try to use non-sparse components with sparse projections (i.e. the wrong learning function or wrong response function). I suggest you make sure you are only using the sparse components and then try again.

Ideally, this is something we would catch before the segfault occurs...

[jlstevens]

Any success with this approach?

[Tasignotas]

Thanks to Jean-Luc and Philipp, the test is now fixed and seems like it's ready to be merged.

[jlstevens]

Looks ready to merge to me!

I'll give Jim a chance to make some final comments before I go ahead.

[philippjfr]

Good work and looks good. Glad this stuff is finally tested, but I'm now wondering whether the sparse test should just be testing against the orginal GCAL test data. Don't think it's worth changing the actual code for this, so maybe just replace the gcal_sparse.ty_DATA with a copy of the original gcal.ty_DATA.

[jbednar]

Looks useful. Is it really necessary to import sparse from topo.submodel.__init__ and topo.submodel.gcal? I'm very concerned that this introduces a dependency on Cython and topo.sparse even for models that don't use any sparse components. Can't this code be moved to a separate file in topo.submodel to avoid polluting the main submodel components?

Also, I don't see why sparsecf is imported as sparseproj, which just seems like it adds confusion.

[jbednar]

I don't have any strong opinion about this, but please consider whether it makes sense to move all the code in these diffs into gcal_sparse.ty itself. It depends what the point of these classes are -- if it is solely for the purpose of testing sparse projections, and if we don't need to test pickling, then it seems cleanest and easiest to have everything in that one .ty file. But if we want to test pickling, or if the new sparse class is something that will be used by other .ty files, then having it in topo.submodel is best, but as a separate file topo.submodel.gcal_sparse rather than in the main gcal.py file.

[jbednar]

In any case, to avoid importing sparse into topo.submodel.__init__, I think you can just replace the code starting "# Register the sheets and projections available in Topographica" with something like the following (untested!) code:

def register_submodel_decorators(classes,superclass=None):
    """
    Register a Model decorator for each of the non-abstract classes provided.
    Only registers those that are subclasses of the given superclass, if specified.
    """
    with param.logging_level('CRITICAL'):
        for c in classes:
            if (isinstance(c, type) and 
                (not hasattr(c, "_%s__abstract" % c.name)) and 
                (superclass==None or issubclass(c, superclass))):
               Model.register_decorator(c)

# Register the sheets and projections available in Topographica
from topo.sheet import optimized as sheetopt
from topo.projection import optimized as projopt
from topo import projection

register_submodel_decorators(topo.sheet.__dict__.values(),topo.sheet.Sheet)
register_submodel_decorators(sheetopt.__dict__.values(),topo.sheet.Sheet)
register_submodel_decorators(projection.__dict__.values(),projection.Projection)
register_submodel_decorators(projopt.__dict__.values(),projection.Projection)

Then the new submodel.gcal_sparse file can just do:

from topo.sparse import sparsecf
register_submodel_decorators(sparsecf.__dict__.values(),projection.Projection)

That way topo.submodel does not need to import sparse, and it's simpler and clearer anyway (assuming it works!).

In any case, once the sparse stuff is in a separate file, please make a clean new pull request with just the new file and any fixes required elsewhere like the above, so that the commit history of this outdated pull request doesn't become part of Topographica.

[jbednar]

Actually, can't these register calls:

 register_submodel_decorators(topo.sheet.__dict__.values(),topo.sheet.Sheet)
 register_submodel_decorators(sheetopt.__dict__.values(),topo.sheet.Sheet)
 register_submodel_decorators(projection.__dict__.values(),projection.Projection)
 register_submodel_decorators(projopt.__dict__.values(),projection.Projection)

be replaced with:

from param import concrete_descendents
register_submodel_decorators(concrete_descendents(topo.sheet.Sheet))
register_submodel_decorators(concrete_descendents(topo.projection.Projection))

This will register decorators for every subclass that has been declared so far for these superclasses, regardless of whether it was in the optimized or non-optimized files. If we do this in the right location in the submodel hierarchy, can we avoid having to do any special handling for sparse projections at all, simply ensuring that they have been imported before this step occurs?

If people don't want to reason about this, that's fine, but if there is a way to do it that's well defined then we'll have fewer problems in the future about decorators not having been defined when we need them.

[jlstevens]

I'm happy to see the mechanism for registering Sheet and Projections with Model improved. As long as the code is tested first (i.e. at least training tests pass) then I am happy see such changes merged.

[Tasignotas]

As suggested by @jbednar, I moved the construction of SparseGCAL test model into gcal_sparse.ty and also improved the decorator registering procedure which works fine for all training tests.

The SparseGCAL currently only switches out the V1_afferent to use SparseCFProjection. The results match the ones produced by the dense GCAL (gcal_sparse.ty_DATA is a copy of gcal.ty_DATA).

[Tasignotas]

Had to rebase the branch. Seems like it should be possible to pull my changes now.

[jlstevens]

Looks great! This PR now addresses just the tests as it should. Personally, I would like to see the GPU PR now, merge that into master then merge these tests.

[philippjfr]

Agreed, this looks good. I think this branch should be merged first though if merely so you don't have to do some complex rebasing to get them in the right order.

[jlstevens]

Ah right. I keep forgetting this isn't GPU tests but sparse tests (which Philipp and I should have added a long time ago!). So yes, this should be merged now and thanks again for doing this!

[jbednar]

The final result looks great, and I am happy for those changes to be made to Topographica.

Philipp, is there any way to cleanly apply the final changes only, so that we don't get the complete history of these changes in Topographica's history? The net result is a very small set of self-contained changes, clearly not disruptive of the rest of the codebase, but the sum of all the individual changes is many times larger and more confusing. So ideally, we'd just get the final set of changes, which would be fine as a single commit. However, if that would cause problems with Ignotas' GPU code, which was based on this branch, we can just bite the bullet and merge this branch as-is. Can you please make that judgement call and either hit Merge on this PR or else create a commit with the net changes and push that? Thanks.

[philippjfr]

I don't want to steal Ignotas attribution for these commits. What I could do is clone his branch, perform an interactive rebase to collapse it into a single commit and then push that but I don't think it's worth the effort and the headaches it'll cause Ignotas. No one is going to go through these commits one by one and look at the changes and in the grand scheme of things 12 commits is nothing. So I'll just go ahead and merge this now.

[Tasignotas]

Thanks!

[jbednar]

That's what I suspected. Thanks for checking and for merging!