Patterncoordinator errata + new Sweeper

[Tobias-Fischer]

This pull request addresses several issues in PatternCoordinator. Furthermore, a new PatternGenerator "Sweeper" for motion is introduced.

Addressed issues in PatternCoordinator

Change inherent_features from a dict to a list

Here, inherent_features has been changed from a dict to a list. If a feature is listed in inherent_features, the corresponding feature coordinator is not added to _feature_coordinators_to_apply. This avoids confusion about the former value of inherent_features, which was only meaningful in PatternCoordinatorImages, but not in PatternCoordinator itself. In PatternCoordinatorImages, a new variable placeholder_mapping has been introduced. This maps a placeholder in the filename_template to its replacement.

Allow supplying a path to a dataset

Rather than supplying a JSON file which specifies an image dataset, this pull request adds the possibility for providing a path to a dataset. By default, all png files within the provided directory are then used. Furthermore, filename_template can now contain glob elements such as * which are evaluated.

Change to FeatureCoordinators

Rather than modifying an existing PatternGenerator, the implementation of FeatureCoordinators has been changed to return a new PatternGenerator. In practise, a copy of the supplied PatternGenerator is made and then modified and returned. This change is neccessary to allow the supplied PatternGenerator to be passed through another PatternGenerator, e.g. a MotionCoordinator might use Sweeper with the supplied PatternGenerator passed to the Sweeper.

Introduce names to numbergen objects for time_dependent=True

When time_dependent=True, the numbergen objects must get passed a name. This has been done here for the present FeatureCoordinators.

Bugfix: use get_value_generator when overwriting parameters

So far, parameters of the PatternGenerators were set as follows: pattern.x += numbergen.UniformRandom(...) This evaluates to pattern.x = pattern.x + numbergen.UniformRandom(...)

However, pattern.x never returns a numbergen object, but always float (even if it is set to a numbergen object).

The correct way to set parameters is instead: pattern.x = pattern.get_value_generator('x') + numbergen.UniformRandom(...)

Sweeper

Sweeper is a more correct implementation of Translator. E.g. if you have a numbergen randomly setting orientation of a Gaussian and you pass it into a Translator, the orientation will keep changing during each sweep phase! This is clearly not what one wants to achieve. Sweeper however allows this behavior. It does so by setting a new time_fn on the supplied pattern generator, which is "slowed down" by dividing by reset_period. Documentation will be added later.

[jlstevens]

Not looked at it properly yet...but the name! SmartSweeper?

Let's just call this class Sweeper and the existing implementation DumbSweeper if you like! Or is it Translator? ;-p

[Tobias-Fischer]

Of course the name needs to be changed! Basically I need to know from you what should happen to the former Sweeper and Translator. Sweeper is used in lissom.ty, and therefore still needs to be present somewhere. Should "SmartSweeper" then replace Translator?

[jlstevens]

If it is just for lissom.ty, I think it would be fine to alias the class to the name Sweeper in the ty file itself.

In addition, I agree that SmartSweeper should replace Translator, though we might need to keep the old class around for legacy reasons.

[jbednar]

Moreover, we don't need to keep lissom.ty running for much longer; it only needs to continue existing in its current form to act as a comparison to the new Model-based approach. Once that all works, and we've verified that it does, we should delete lissom.ty and never look back. Thus if it's helpful to move some code needed only by lissom.ty into lissom.ty itself, we can do that. We might have some tests depending on lissom.ty that would be affected, but I guess we can deal with that if so.

Jim

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

[Tobias-Fischer]

As Jim suggested, I removed the old Sweeper class from ImaGen. Because the old Sweeper class only has been used in lissom.ty, I moved the code in lissom.ty (see https://github.com/ioam/topographica/pull/570).

[jlstevens]

These all look like minor tweaks to me that could be committed directly to master in a series of small commits (instead of via a PR).

None of the things mentioned above look important enough to hold up Tobias' project!

Maybe we should come back to these issues later?

[jbednar]

I think these can all be dealt with today, though, to be done with them, so that we don't have to revisit them. Because Tobias doesn't have commit access, it has to be through pull requests, which tends to lump them...

[jlstevens]

In that case, I'll let Tobias fix these changes tonight and then I'll merge (i.e. any new changes after this should be done on master).

[jlstevens]

I'm glad this is merged!

I do want to change MANIFEST_json filename to something that looks a little less ridiculous. I'm happy to add a special case to allow 'MANIFEST.json' to be used (at some point when I get round to it!).

[jlstevens]

I do have one outstanding problem with this class (and a few others in Imagen) - see Issue #35 .