Changes required to replicate results in my thesis

[Tobias-Fischer]

This PR consists of several smaller changes, with the biggest change being the introduction of code for the new color model.

The changes are as follows: 1) When 'dr' is used, the position_bound is increased. This makes sure the pattern is swept across the connection fields, even for neurons at the edge. 2) A switch to use image datasets is introduced. Please note that the standard McGill images are not compatible. Instead, images with 3 channels have to be used with the new color model. 3) Introduction of code for the new color model. 4) Add possibility of gain control for the spatial frequency channels. I had some more tweaks for my thesis, however that was more playing around with different extents of the gain control projections etc.; however, there was no scientific reasoning for that so I left them out. Spatial frequency needs further work anyway.

Please note that some of the changes are a bit hackish (although I tried to make them the least hackish as I could). If you think they are too hackish and it's easier for you to change than for me, please feel free to make the required changes yourself outside this pull request.

[jbednar]

Doesn't this code depend on changes to FeatureMapper not yet committed? E.g. the various NewRetina color metaparams?

[Tobias-Fischer]

Yes .. I will create pull requests for that tomorrow.

[jbednar]

When you do, you'll need to update this PR, because it's referring to objects in featuremapper like ChrisBallNewRetina, which sounds more like a medical procedure for our good friend than something we can safely use in Topographica. If there needs to be a very specialized new metaparam type, it could be named after the publication which introduces it (ball_phd14, in this case). But certainly our goal with your project and Giacomo's was to be able to make the "new" retina and "new" motion model be the default and usual one, with the older support either tucked away unobtrusively somewhere or eliminated. So I wouldn't want the new stuff to end up with names like that!

[Tobias-Fischer]

For now, I kept the "NewRetina" in the name (but removed ChrisBall in the name). The credits to the code in featuremapper I am referring to go to Giacomo. In the e-mail he sent me the code he also mentioned that he has some code which checks whether multichannel objects are used. Using this code, the classes for the new retina could be merged with the already existing classes. I hope Giacomo can finalize his draft code, so that there won't be additional classes for the multichannel retina anymore.

[Tobias-Fischer]

Please note that this PR depends on https://github.com/ioam/imagen/pull/37 and https://github.com/ioam/featuremapper/pull/11 to be merged first.

[jlstevens]

The first thing I want to know - do the GCAL training tests still pass?

I'm not happy to see all these changes to earlyvision.py and gcal.py as they are bound to cause me trouble!

[Tobias-Fischer]

How can I test this?

[jlstevens]

Run the training tests with topographica -t training.

By the way, I think colour is an absolute nightmare - I want absolutely nothing to do with it!

Is there any way we can have GCAL inherit from EarlyVisionModel and ColorGCAL inherit from ColorEarlyVisionModel? Otherwise, there seems no point having the two EarlyVisionModel classes if the colour mess has to be inherited by all superclasses.

[jlstevens]

Having spent weeks getting ModelGCAL clean, I am not at all happy to merge this in. Most of these changes seem to relate to color which:

1. Is a mess both biologically and in the model implementation (unavoidably so!).
2. Is a poorly understood feature with very confusing, self-contradictory literature (from what I understand).
3. Complicates the definition of the model which would otherwise handle all the other features just fine.

I am very happy that ColorEarlyVisionModel is separate from EarlyVisionModel but I am not at all happy that (by default) GCAL inherits the godawful mess that is colour vision! Note that ChannelGeneratorSheet should not be in EarlyVisualModel but in ColorEarlyVisionModel.

For this reason, I propose that ModelGCAL inherits from EarlyVisionModel and that ModelColorGCAL inherits from ColorEarlyVisionModel. That way sane people never have to know that colour vision even exists - as it should be! ;-p

I would be much happier if this PR only added the improvements to EarlyVisionModel and ModelGCAL that don't relate to colour. A separate PR could then address colour separately.

[jbednar]

This is precisely why we need to have a modular, not just inheritance-based, architecture. We've already gone over why this is difficult to achieve in general, but all we really need is to have a break between the LGN and V1, so that any cortical model can be attached to any model providing an LGN (and preferably to any other cortical model). Having a ModelColorGCAL seems like a stopgap measure that we may need to do, before we get a chance to do it properly.

[jlstevens]

I've discussed this with Jim and we agree all colour related concepts should go into ColorEarlyVisionModel and ModelColorGCAL (or is it ModelGCALColor?).

As Jim has pointed out above, there is a real issue with modularity here (that we will need to think about in future!) but this is not something Tobias needs to worry about now.

The improvements that I agree should be in ModelGCAL:

• Addition of the datasets parameter.
• Use of PatternCoordinatorImages.

I've read your summary about the position bound for 'dr'. I can see why motion_buffer exists but it does seem like another annoying wrinkle. Other than that, everything else looks colour related to me.

[Tobias-Fischer]

The training tests still pass with the changes above. However, the corresponding imagen PR (https://github.com/ioam/imagen/pull/37) causes the training tests to fail, as all Gaussian patterns are oriented the same way in order to get biologically realistic direction maps.

[Tobias-Fischer]

Please let me know as soon as you finally decided what should happen with this PR. I am happy to create a separate PR only containing the non-color related changes if you want. The color-related stuff could then be integrated as soon as a modular model structure is possible.

[jlstevens]

I am happy to create a separate PR only containing the non-color related changes if you want.

That would be great and I do appreciate the effort you've put into doing this properly!

Edit: We might want to keep the change that breaks the tests as a minimal, independent commit to make sure everything else works before introducing a test-breaking change.

[jbednar]

You'll probably just toss this PR once the non-color one has been merged, but I think it's basically ok at that point, modulo my recent comments on this one.

[Tobias-Fischer]

Yeah, I am not planning to work on this PR. It seems like it's best to wait with the color related changes until a modular model structure is possible. Once that's done, some rework has to be done anyway, and the changes you suggest shouldn't take too long to do then. What should happen with this PR in the meantime? Should this be an issue? How can we ensure the code is kept somewhere it can be found later on?

[jbednar]

I think we should go ahead and merge it, but copying ModelGCAL to ModelColorGCAL as Jean-Luc suggested (temporarily!!!), and then making ModelGCAL inherit from EarlyVisionModel, while ModelColorGCAL inherits from ColorEarlyVisionModel. That way, we can have ModelGCAL be simple and clean and easily understood (not having to trust that ColorEarlyVisionModel works like EarlyVisionModel for non-color simulations). Then we'll be able to apply all of your pull requests safely, improve them ourselves, while still being able to replicate your thesis. But of course that won't work in the long run, because ModelColorGCAL would then contain tons of things duplicating ModelGCAL, which is a recipe for disaster and confusion. So at that point we'll work in breaking the link between the EarlyVision models and the GCAL models, so that any GCAL model can use any EarlyVision model, as needed in a particular .ty file (presumably by having the ModelGCAL class instantiate an EarlyVision class for it to use). But that will be long after you've gone, and so you don't need to worry about it. Meanwhile, can you please just use a ModelColorGCAL class copied verbatim from ModelGCAL (in a new file just to do with color, perhaps) so that we can capture all of your code and results from your thesis now, before you disappear and we lose all chance of ever recreating it (since you don't even have the exact parameters you used for us to have as reference)? Please, please?

[jbednar]

Sorry -- to clarify -- I think we should merge this code, not necessarily this PR! It probably makes sense to have a new, separate PR with just the changes needed.

[Tobias-Fischer]

I created a new pull request: https://github.com/ioam/topographica/pull/588

As soon as the other PR is merged, this PR can be closed.