Added support for NChannelGeneratorSheet and optimized color conversion

[spiglerg]

Optimized colorconversion added to topo.optimized.color (new file) and topo.analysis.__init (overriding featuremapper.colorspaces default unoptimized functions).

NChannelGeneratorSheet added to topo.base.generatorsheet.

[jlstevens]

Mostly looks fine...though this is the first time I've seen a HACKATTACK alert!

[spiglerg]

Warning: if we transition to using CompositeImage to handle monochrome-generators but not to wrap NChannels in general (for the channel scaling thing, mostly), then all its "hacky" logic which was used to get to the top-most generator ( to access ._channel_data ), will have to be copied into the NChannelGeneratorSheet, too, because otherwise it will not work with "Composite" inputs (the proper ones, not CompositeImage) Plus, it would be copied both into set_input_generator (to figure out whether the input is multichannel and how many channels does it have [in order to properly set the src_ports and setup the data structures]) and into the generate() method, to copy the data.

[jlstevens]

I'll need to think a bit about this. For now, I'll link to the Imagen PR (and link back) so we can easily jump to the code we are talking about...

EDIT: I believe Jim and Chris were considering implementing a method to make this less of a hack...

[jlstevens]

I have to admit that I'm a little confused!

Shouldn't the generator sheet simply ask the supplied generator for whatever array it requires (either via call or via channels)? I can understand why CompositeImage needs to dig inside composite PatternGenerators but why would a GeneratorSheet have to to this? Maybe there is some more hackery involving GeneratorSheets that I don't know about?

The only place I see where the hack is needed (to get the appropriate generator) is in CompositeImage - the Image is defined by several other pattern generators.

I guess I must be forgetting something...

[spiglerg]

Right now NChannelGeneratorSheet takes a CompositeImage as input (in the current implementation, that is), and the CompImg is NChannel: it takes generators (NChannel and monochrome) and copies them into its own "channel_data" field.

NChannelGeneratorSheet might take different generators as input, though:

• if it is monochrome, it should behave as a monochrome generatorsheet
• if it is FileImage or CompositeImage, it will behave as NChannel
• if it is, like usual, a pattern.Selector (eg, images in a dataset), it will think that it's monochrome, because it doesn't have a "channel_data" field, even though it could actually be created out of FileImage
  • right now, RGBImage (that is, FileImage) are loaded and wrapped into a pattern.Selector, which in turn is wrapped into ExtendToNChannel (CompositeImage), which in the end provides the NChannel interface. This is why it works now, despite using pattern.Selector, and why the system won't work if Selector is not wrapped into multichannel structures (OR the objects that access it have a way to locate the current_generator and access its channel_data)

Hope this is a bit clear, though it still look messy. :P

On Tue, Jul 15, 2014 at 8:42 PM, J.L Stevens notifications@github.com wrote:

I have to admit that I'm a little confused!

Shouldn't the generator sheet simply ask the supplied generator for whatever array it requires (either via call or via channels)? I can understand why CompositeImage needs to dig inside composite PatternGenerators but why would a GeneratorSheet have to to this? Maybe there is some more hackery involving GeneratorSheets that I don't know about?

The only place I see where the hack is needed (to get the appropriate generator) is in CompositeImage - the Image is defined by several other pattern generators. I guess I must be forgetting something...

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49082380.

[jlstevens]

Ok. Unfortunately at this point everything seems quite convoluted and I am a bit lost. I'm sure there is a way to keep everything simple but I no longer understand the requirements well enough to suggest how. Maybe Jim can suggest whether he thinks this situation can be simplified or whether there really is a fundamental problem here?

[jbednar]

Unfortunately, I don't see how to make this work cleanly without adding channels() to the Selector pattern.

Essentially, what Giacomo is pointing out is that the NChannelGeneratorSheet needs to have access to the underlying channels, and unless Selector (and mabye Composite?) makes those available directly, either NChannelGeneratorSheet has to do fancy logic to find the underlying multiple channels, or else we have to wrap the PatternGenerator in something that has that fancy logic itself.

The obvious solution would be to add channels and channel support to the Selector pattern, simply promoting any channels of the chosen sub-generator that might be available to be its own channels. This seems like a small amount of reasonably clear code, and is in the spirit of ImaGen really being about (multichannel) images.

I'm not sure whether Composite also would need such support, but possibly not -- our existing Numpy-derived ufunc operators aren't particularly well-defined if considering multiple channels.

So, any objections to adding this support to at least Selector?

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

[jlstevens]

Ah ok... I understood something along those lines.

In short, I agree - as soon as we have the concept of Images and channels (and it is called ImaGen after all!) then this concept should be used consistently everywhere. For normal pattern generators (e.g. Gaussian) channels with be a list of one array which is redundant with the output of call. This can be implemented as a trivial change to the PatternGenerator base class.

After that, there is Selector and various composite types - they need to also implement channels in whatever way makes sense as a function of their input. For Selector at least, this is fairly simple and corresponds exactly to what you suggest. For other composite types, the semantics may need some more thought...

[spiglerg]

I agree it would be a good solution, but currently we rely on the existence of the "channels" attribute to determine whether the object supports multiple channels. Should the Selector's channels() return None if the generators it wraps are not multichannel? And then NChannelGeneratorSheet and CompositeImage would both check for hasattr(generator, 'channels') and "generator.channels is not None" ?

I mean, is there a way in python to "remove" the atribute if it is not NChannel?

For Jean-Luc: what do you mean exactly? Right now the default behavior is that monochrome channels use the mechanisms already in the repository, and "channels" only contain data if the number of channels is >1 . Eg, I wouldn't fill in channels with a single array if the image was monochrome. Eg2, if you load a monochrome with GenericImage it won't fill in "channels" (see the updated pull request)

G

On Wed, Jul 16, 2014 at 12:23 AM, James A. Bednar notifications@github.com wrote:

Unfortunately, I don't see how to make this work cleanly without adding channels() to the Selector pattern.

Essentially, what Giacomo is pointing out is that the NChannelGeneratorSheet needs to have access to the underlying channels, and unless Selector (and mabye Composite?) makes those available directly, either NChannelGeneratorSheet has to do fancy logic to find the underlying multiple channels, or else we have to wrap the PatternGenerator in something that has that fancy logic itself.

The obvious solution would be to add channels and channel support to the Selector pattern, simply promoting any channels of the chosen sub-generator that might be available to be its own channels. This seems like a small amount of reasonably clear code, and is in the spirit of ImaGen really being about (multichannel) images.

I'm not sure whether Composite also would need such support, but possibly not -- our existing Numpy-derived ufunc operators aren't particularly well-defined if considering multiple channels.

So, any objections to adding this support to at least Selector?

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

—

Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49105968.

[jbednar]

There are some tricky issues here, but here's what I think we should do if we are really adding channels support at the top level of PatternGenerator:

• Implement channels() as a method call, not an attribute or property, to match the existing call semantics of a PatternGenerator.
• Make the default implementation of channels() at the PatternGenerator level be to run call() and return the result as a list of channels with that one array in it.
• Allow any PatternGenerator to override channels() if it wishes, such as for an Image.
• Code that uses PatternGenerators would then either use the simpler call() interface, which will always return a single array, or the new channels() interface, which will return at least one and possibly more arrays. No code needs to use both of these interfaces, and indeed it's simplest never to do so, to avoid confusion. E.g. GeneratorSheet could use call() exclusively, and ChannelsGeneratorSheet could use channels() exclusively.
• For e.g. RGB images, there is an ambiguity about whether the channels list should also include the averaged (grayscale) array or not, presumably as the first channel (channel 0). By default it would not -- if using the channels interface, presumably you want the channels, not any averaged version! But if we found that keeping the channels and the single-array versions separate like that made ChannelsGeneratorSheet too difficult, or made it hard to implement code that works well for both monochrome and RGB images, then it seems fine to add the averaged array as the first channel.
• For generality, it seems to me like the channels list ought to be an ordered dictionary, so that we can easily access the channels by name or by index.

Sound ok?

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

[jbednar]

Oh, and I favor not implementing any channels support for Composite for now; we can always add that later.

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

[spiglerg]

So, in practice, the support is to be added to PatternGenerator directly, and will return the grayscale by default (or better, an array with a single element, the result of call --> in this way it's easy to check the generator's support for NChannels) ?

For RGB, I'd say that grayscale is not technically a channel of the image, it's just a way to make it compatible with other objects that might want to use PatternGenerators.

G

On Wed, Jul 16, 2014 at 10:43 AM, James A. Bednar notifications@github.com wrote:

There are some tricky issues here, but here's what I think we should do if we are really adding channels support at the top level of PatternGenerator:

• Implement channels() as a method call, not an attribute or property, to match the existing call semantics of a PatternGenerator.
• Make the default implementation of channels() at the PatternGenerator level be to run call() and return the result as a list of channels with that one array in it.
• Allow any PatternGenerator to override channels() if it wishes, such as for an Image.
• Code that uses PatternGenerators would then either use the simpler call() interface, which will always return a single array, or the new channels() interface, which will return at least one and possibly more arrays. No code needs to use both of these interfaces, and indeed it's simplest never to do so, to avoid confusion. E.g. GeneratorSheet could use call() exclusively, and ChannelsGeneratorSheet could use channels() exclusively.
• For e.g. RGB images, there is an ambiguity about whether the channels list should also include the averaged (grayscale) array or not, presumably as the first channel (channel 0). By default it would not -- if using the channels interface, presumably you want the channels, not any averaged version! But if we found that keeping the channels and the single-array versions separate like that made ChannelsGeneratorSheet too difficult, or made it hard to implement code that works well for both monochrome and RGB images, then it seems fine to add the averaged array as the first channel.
• For generality, it seems to me like the channels list ought to be an ordered dictionary, so that we can easily access the channels by name or by index.

Sound ok?

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

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49142787.

[spiglerg]

In fact, in my current code the default is handled by checking the length of the channel list: if it is empty, it's monochrome.

On Wed, Jul 16, 2014 at 1:17 PM, Giacomo Spigler spiglerg@gmail.com wrote:

So, in practice, the support is to be added to PatternGenerator directly, and will return the grayscale by default (or better, an array with a single element, the result of call --> in this way it's easy to check the generator's support for NChannels) ?

For RGB, I'd say that grayscale is not technically a channel of the image, it's just a way to make it compatible with other objects that might want to use PatternGenerators.

G

On Wed, Jul 16, 2014 at 10:43 AM, James A. Bednar < notifications@github.com> wrote:

There are some tricky issues here, but here's what I think we should do if we are really adding channels support at the top level of PatternGenerator:

• Implement channels() as a method call, not an attribute or property, to match the existing call semantics of a PatternGenerator.
• Make the default implementation of channels() at the PatternGenerator level be to run call() and return the result as a list of channels with that one array in it.
• Allow any PatternGenerator to override channels() if it wishes, such as for an Image.
• Code that uses PatternGenerators would then either use the simpler call() interface, which will always return a single array, or the new channels() interface, which will return at least one and possibly more arrays. No code needs to use both of these interfaces, and indeed it's simplest never to do so, to avoid confusion. E.g. GeneratorSheet could use call() exclusively, and ChannelsGeneratorSheet could use channels() exclusively.
• For e.g. RGB images, there is an ambiguity about whether the channels list should also include the averaged (grayscale) array or not, presumably as the first channel (channel 0). By default it would not -- if using the channels interface, presumably you want the channels, not any averaged version! But if we found that keeping the channels and the single-array versions separate like that made ChannelsGeneratorSheet too difficult, or made it hard to implement code that works well for both monochrome and RGB images, then it seems fine to add the averaged array as the first channel.
• For generality, it seems to me like the channels list ought to be an ordered dictionary, so that we can easily access the channels by name or by index.

Sound ok?

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

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49142787.

[jbednar]

I'm not sure what you mean about checking the generator's support for channels; all generators would then support channels. Some might only return one channel, of course.

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

[spiglerg]

Yeah, I meant that. Right now I check if a generator supports channels by probing for "channel_data" or alike. With the new modifications, 'channels' will always be supported (well, not always, see Composite; the check is still useful) but it will usually return 1-elements lists.

On Wed, Jul 16, 2014 at 1:20 PM, James A. Bednar notifications@github.com wrote:

I'm not sure what you mean about checking the generator's support for channels; all generators would then support channels. Some might only return one channel, of course.

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

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49157175.

[spiglerg]

\ sorry, if Composite extends PatternGenerator it will have 'channels' too!

On Wed, Jul 16, 2014 at 1:21 PM, Giacomo Spigler spiglerg@gmail.com wrote:

Yeah, I meant that. Right now I check if a generator supports channels by probing for "channel_data" or alike. With the new modifications, 'channels' will always be supported (well, not always, see Composite; the check is still useful) but it will usually return 1-elements lists.

On Wed, Jul 16, 2014 at 1:20 PM, James A. Bednar <notifications@github.com

wrote:

I'm not sure what you mean about checking the generator's support for channels; all generators would then support channels. Some might only return one channel, of course.

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

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49157175.

[jbednar]

Right -- Composite will have channels; it will just not (currently) do much useful for multi-channel images, because it will only provide a single channel. We can add a note to its documentation saying so for now, and then later make it do something useful for multi-channel inputs once we've decided what that should be (presumably after your project is done!). I don't think the channels list should be empty for a monchrome image (or for any pattern ever!); it should just have one channel.

[spiglerg]

Exactly!

Well yes, in the new implementation of channels() it will return the monochrome as a single element.

Also, Selector's channels() should return the get_current_generator().channels(), right?

On Wed, Jul 16, 2014 at 1:24 PM, James A. Bednar notifications@github.com wrote:

Right -- Composite will have channels; it will just not (currently) do much useful for multi-channel images, because it will only provide a single channel. We can add a note to its documentation saying so for now, and then later make it do something useful for multi-channel inputs once we've decided what that should be (presumably after your project is done!). I don't think the channels list should be empty for a monchrome image (or for any pattern ever!); it should just have one channel.

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49157568.

[jlstevens]

Just to say I have no conceptual problem with always having one or more channels. Just check if len(channels) > 1 for 'NChannel'. For our usual pattern generators, there is only one channel (the same as the output of call) and for some of our new classes, there will be multiple channels.

The semantics are fairly clear too: call always returns a single array and channels contains the various components (one or more as applicable). I think I'm probably just reiterating (and hopefully further clarifying!) what Jim has already said above...

[spiglerg]

Agreed!

On Wed, Jul 16, 2014 at 1:32 PM, J.L Stevens notifications@github.com wrote:

Just to say I have no conceptual problem with always having one or more channels. Just check if len(channels) > 1 for 'NChannel'. For our usual pattern generators, there is only one channel (the same as the output of call) and for some of our new classes, there will be multiple channels.

The semantics are fairly clear too: call always returns a single array and channels contains the various components (one or more as applicable).

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49158365.

[jbednar]

Yes, I think Selector.channels() should return get_current_generator().channels().

[spiglerg]

I've committed the addition of channels(). Now I'm going to adapt NChannelGeneratorSheet (topographica pull request) to support it too!

On Wed, Jul 16, 2014 at 1:45 PM, James A. Bednar notifications@github.com wrote:

Yes, I think Selector.channels() should return get_current_generator().channels().

— Reply to this email directly or view it on GitHub https://github.com/ioam/topographica/pull/569#issuecomment-49159661.

[spiglerg]

Re-pulling my code changes.