holoviz-topics / imagen

ImaGen: Generic Python library for 0D, 1D and 2D pattern distributions
https://imagen.holoviz.org/
BSD 3-Clause "New" or "Revised" License
31 stars 16 forks source link

Added PatternCoordinator class which returns a set of coordinated pattern generators #24

Closed Tobias-Fischer closed 10 years ago

Tobias-Fischer commented 10 years ago

A PatternCoordinator is a set of coordinated pattern generators. The pattern generators are named according to the labels parameter. The dimensions to be returned are specified with the dims parameter. PatternGenerators are first instantiated with the parameters specified in input_parameters, and then subclasses of ValueCoordinator are applied to modify the properties of these generators.

Tobias-Fischer commented 10 years ago

Once the pull request is confirmed, Topographica will be changed to support PatternCoordinator in the following ways:

  1. a new file topo/misc/metafeatures.py will be created which contains further ValueCoordinator subclasses
  2. in topo/init.py the metafeature_mapping will be updated:
from topo.misc.metafeatures import VC_ocularity, VC_disparity
imagen.patterncoordinator.PatternCoordinator.metafeature_mapping.update({'od': VC_ocularity,'dy': VC_disparity})

Also, import statements will be added in the same way as imagen.image and imagen.random are imported.

import imagen.random, imagen.image, imagen.patterncoordinator
sys.modules['topo.pattern.patterncoordinator']=pattern.patterncoordinator
Tobias-Fischer commented 10 years ago

The last three commits should incorporate the comments from last night. Please let me know if there is anything more you want to get changed :).

Tobias-Fischer commented 10 years ago

Please also update the submodule reference to Param (here as well as in topographica) because both param_keywords() and the new ClassSelector are used.

jbednar commented 10 years ago

Ok, it's really getting there now! I wasn't able to see the true structure until you implemented the changes from last night, but I do think it's converging now, and that my suggested new new names will greatly simplify it for users and make it easier to follow, with fewer redundant concepts and confusing terms. I hope you agree, and don't just think I'm wasting your time! :-)

Tobias-Fischer commented 10 years ago

I definitely see that the structure has improved a lot. Now I have to get rid of the old names and new names, and stick with the hopefully final new new names :-). Thanks a lot for your comments!

jlstevens commented 10 years ago

Hi

I hope you don't mind me taking a peak at this PR and making a quick comment about the proposed class names! :-)

The name FeatureCoordinator sounds good to me. When I first heard about it, I assumed you would then be using names like XCoordinator, OrientationCoordinator for specific features. Of course these full names are rather long but they are clear - I'm not enamored with the FC_<X> convention.

Maybe you have a strong reason to keep the names as short as possible? If this is the case then the proposed names are probably be as good as any. Otherwise, I think it would be good to make sure no one can think of any better suggestions before merging this PR!

Edit: Sorry, I just read your comment above - you seem to be changing names already - I look forward to seeing the new suggestions!

jbednar commented 10 years ago

I don't think there is any reason for the names to be short; most people won't ever use these functions directly, but via their short string names "xy" or "orientation". So I'm happy with XCoordinator or OrientationCoordinator if you prefer that. And yes, do please weigh in on things like this; it's been hard to get all the names and detailed semantics right!

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

jlstevens commented 10 years ago

I know this PR has just been merged but I would like to make confirm that we will be using time_dependent=True in FeatureCoordinators!

Tobias-Fischer commented 10 years ago

Yes, we will be doing that. That and probably some other small changes will be implemented as soon as we have FeatureCoordinators for color and motion.

jbednar commented 10 years ago

We eventually plan to use time_dependent=True, but it is best to hold off for now, because we want to be able to move the existing .ty files to PatternCoordinators without changing their results. Once that's done and tested, I will be very happy for everything to move to time_dependent=True and not look back!

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