Added support for NChannelImage and ExtendToNChannel

[spiglerg]

Sorry for all the previous open/close pull requests. This is the only merge to look at.

[jlstevens]

Travis still isn't finding your PR for some reason... maybe we should wait a bit longer?

[spiglerg]

Travis still isn't working..

[spiglerg]

Though I've just checked locally and it executes fine now!

[jlstevens]

I've added some comments above. I'm fairly uncomfortable with the number of times the word 'hack' appears in this diff!

Edit: The word 'hack' appears in 7 different places!

[spiglerg]

I know, but mind 90% of this code is taken from Chris and adapted. It was developed over years of "hacking" through to make it work with very specific instances.

I'd suggest to integrate it into Imagen, and keep it "secret" for a while until it's made perfect.

Right now I'm in a huge mess keeping track of all these files and pull requests, and I'd really need to have this code integrated into "stable placements" as soon as possible, so I can convert all my thesis code to support the new objects. :/ I cannot proceed on with my dissertation until all this is sorted out, which is a huge bottleneck..

On Thu, Jul 10, 2014 at 2:30 PM, J.L Stevens notifications@github.com wrote:

I've added some comments above. I'm fairly uncomfortable with the number of times the word 'hack' appears in this diff!

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48604048.

[spiglerg]

Fixed some stuff, and now Travis is working ;)

[jlstevens]

None of my suggestions above will take much work to fix. Removing the commented code shouldn't be hard and neither is replacing the assert with a decent exception. Renaming the method _again to something sane shouldn't take long either. In total this should take no more than 10 minutes work.

Unfortunately, the actual hacks used are a more fundamental problem we can't address right now.

[spiglerg]

Yeah sure, I didn't meant that. ;) I've fixed everything but the assert, still thinking of the best way to replace it.

On Thu, Jul 10, 2014 at 2:39 PM, J.L Stevens notifications@github.com wrote:

None of my suggestions above will take much work to fix. Removing the commented code shouldn't be hard and neither is replacing the assert with a decent exception. Renaming the method _again to something sane shouldn't take long either. In total this should take no more than 10 minutes work.

Unfortunately, the actual hacks being used are a more fundamental problem we can't address right now.

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48605063.

[jlstevens]

As a general comment the methods and parameters are lacking docstrings. I'll let Jim decide if he wants to merge before having them added.

[jlstevens]

To be honest, I don't know enough about the RGBImage class to say where it should go. Best ask Jim about it. That said, it sounds like an Imagen thing to me!

Unfortunately, the Travis tests are failing again. Apart from that, I think I've made all the comments I want to about this PR (for now!).

[spiglerg]

It would be imagen, but it depends on featuremapper.colorspaces, so putting it into Imagen would broke the dependencies chain. :P

Are they? I'll have a look and fix now!

On Thu, Jul 10, 2014 at 3:26 PM, J.L Stevens notifications@github.com wrote:

To be honest, I don't know enough about RGBImage to say where it should go. Best ask Jim about it. That said, it sounds like an Imagen thing to me!

Unfortunately, the Travis tests are failing again. Apart from that, I think I've made all the comments I want to about this PR (for now).

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48611357.

[spiglerg]

Oh crap, I've committed old code.. I have too many copies of this code, as I need separate files to run the old code I need for my thesis, until it's all integrated... It's becoming a huge mess. :(

On Thu, Jul 10, 2014 at 3:27 PM, Giacomo Spigler spiglerg@gmail.com wrote:

It would be imagen, but it depends on featuremapper.colorspaces, so putting it into Imagen would broke the dependencies chain. :P

Are they? I'll have a look and fix now!

On Thu, Jul 10, 2014 at 3:26 PM, J.L Stevens notifications@github.com wrote:

To be honest, I don't know enough about RGBImage to say where it should go. Best ask Jim about it. That said, it sounds like an Imagen thing to me!

Unfortunately, the Travis tests are failing again. Apart from that, I think I've made all the comments I want to about this PR (for now).

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48611357.

[jlstevens]

Frankly, I don't see why colorspaces doesn't go into Imagen in the first place. It looks useful, generic and suitable for Imagen. I assume you have some good reasons to put it in featuremapper?

If not, I suspect RGBImage and colorspaces should be in Imagen instead of spreading these classes all over the place.

[spiglerg]

Hmm we just thought that Imagen should have stayed more general, only caring about generating "arrays of data" rather than images.

RGBImage is a bit more Topographica as it natively supports hue rotation, which is useful for our scopes. However it also adds support for loading images, which is useful in Imagen as well.

On Thu, Jul 10, 2014 at 3:32 PM, J.L Stevens notifications@github.com wrote:

Frankly, I don't see why colorspaces doesn't go into Imagen in the first place. It looks useful, generic and suitable for Imagen. I assume you have some good reasons to put it in featuremapper?

If not, I suspect RGBImage and colorspaces should be in Imagen instead of spreading these classes all over the place.

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48612273.

[jlstevens]

Ok. As long as you have carefully considered the pros/cons of adding both colorspace and RGBImage to Imagen.

Given that imagen already has an image module and support for things like FileImage I would say imagen already does support images. I wouldn't say that hue rotation is such a specific Topographica thing either - it is a general operation that anyone working with images will understand (it has nothing to do with neuroscience per se!).

I am in favor of pushing as much functionality down (or is it up?) to our most basic dependencies (param, imagen) where possible. That way we make as much functionality available to non-neuroscientists as we can!

Of course, Jim understands the details better than I do and will have the final say on this issue. I don't want him to accuse me of leading you astray! :-)

[spiglerg]

Sounds reasonable!

I'll write to Jim and ask for comments.

In the meanwhile, I'm almost ready to make the Pull Request to Topographica with everything else that I need, except RGBImage!

On Thu, Jul 10, 2014 at 3:47 PM, J.L Stevens notifications@github.com wrote:

Ok. As long as you have carefully considered the pros/cons of adding both colorspace and RGBImage to Imagen. Given that imagen already has an image module and support for things like FileImage I would say imagen already does support images. I wouldn't say that hue rotation is such a specific Topographica thing - it is a general operation that anyone working with images will understand (it has nothing to do with neuroscience per se!).

Of course, Jim understands the details better than I do and will have the final say on this issues. I don't want him to accuse me of leading you astray! :-)

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48614413.

[spiglerg]

I've moved the random_numbergen param into RGBImage. It was nonsense to have it inside NChannelImage, which doesn't support hue rotation. ;)

On Thu, Jul 10, 2014 at 3:52 PM, Giacomo Spigler spiglerg@gmail.com wrote:

Sounds reasonable!

I'll write to Jim and ask for comments.

In the meanwhile, I'm almost ready to make the Pull Request to Topographica with everything else that I need, except RGBImage!

On Thu, Jul 10, 2014 at 3:47 PM, J.L Stevens notifications@github.com wrote:

Ok. As long as you have carefully considered the pros/cons of adding both colorspace and RGBImage to Imagen. Given that imagen already has an image module and support for things like FileImage I would say imagen already does support images. I wouldn't say that hue rotation is such a specific Topographica thing - it is a general operation that anyone working with images will understand (it has nothing to do with neuroscience per se!).

Of course, Jim understands the details better than I do and will have the final say on this issues. I don't want him to accuse me of leading you astray! :-)

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48614413.

[jlstevens]

Ok - sounds good.

That said, I prefer random_distribution or random_jitter as a name - random_numbergen was my least favorite suggestion! You can pick a better name than me because you know exactly what it is for...

[spiglerg]

Hmm ok!

On Thu, Jul 10, 2014 at 4:20 PM, J.L Stevens notifications@github.com wrote:

Ok - sounds good.

That said, I prefer random_distribution or random_jitter as a name - random_numergen was my least favorite suggestion! You can pick a better name than me because you know exactly what it is for...

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-48619141.

[jbednar]

Sorry that I've been busy 24/7 the past few days and haven't been able to stay on top of this! The RGB image loading must go into imagen. The hue rotation is optional; it doesn't make a lot of sense for imagen, but as Jean-Luc says, it does make some sense. I had suggested splitting RGBImage into two classes, one doing the image loading (and going into imagen) and the other doing the hue rotation (and going into something higher, either featuremapper or topographica). But if it is really holding you back with your project, and if you agree that colorspaces can go into imagen, then just put it and RGBImage there and if necessary we can move it later, rather than trying to piece it out carefully now.

[jlstevens]

I'll stop making comments now so that spiglerg can make progress on this other work. ;-p

I will say that the nameExtendToNChannel is fairly horrible. It sounds like some auxiliary utility and not like some fundamental component that belongs in patterngenerator.py.

Why not call it MultiChannel and be done with it? This name seems fairly obvious given that the docstring states that it exists ''to support multiple channels''.

EDIT: I couldn't help but make a few more comments below. These are things we should keep in mind for later and aren't reasons to hold up the merge.

[jlstevens]

To reply to Jim's comment, I think there it is perfectly reasonable to leave all this functionality in imagen as it is all fairly general (though a little bit of renaming is required).

Splitting out tiny bits of functionality across imagen/featuremapper seems like it will be more hassle than it is worth!

[jlstevens]

I started by writing an objection about the terrible names (NChannelWrapper is quite horrible as is ExtendtoNChannel) before coming to the conclusion that this is a consequence of the nonsense organization of this whole file. Now I find pretty much everything in imagen.image objectionable - and now it is only going to get worse! :-(

For a start, having multiple (typically 3 or 4) channels in an Image is not a special case - it is what everyone expects (by default people expect images to have RGB or RGBA support, supporting more should be a basic additional feature of imagen as a scientific tool).

Ideally, this would have been designed in from the start because now I find every name currently violates my expectations for what the class should do. Here is what we have now:

    PatternGenerator -- >  GenericImage -- > FileImage
                                |-      -- > NumpyFile

This all wrong! GenericImage is not generic (it doesn't support N-channels, i.e. a basic feature) and FileImage doesn't support all files (just image files supported by PIL).

Here is the new proposal (which is even more silly!)

    PatternGenerator -- >  GenericImage -- > FileImage
                                |-      -- > NChannelImage --> RGBImage

What is wrong with this?

• GenericImage is not generic. It is monochrome only.
• FileImage only supports file formats supported by PIL - the name sounds more general than it is. It would be more honest to call it PILImage
• NChannelImage is constrained to numpy files that may or may not have multiple channels. The fact that it has N-channels is irrelevant (this is basic functionality for images) but the fact that it only reads in .npy files is relevant. It should be called NumpyImage.

All the names are confusing nonsense imho. Here is how it should be:

PatternGenerator -- >  Image -- > FileImage

• Image replaces GenericImage (with multi-channel support by default). This is an abstract (base) class. I don't mind leaving it as GenericImage if you are worried about a name clash with PIL.Image.
• FileImage reads in any supported file type (supported PIL formats and .npy files).

Instead of NChannelWrapper we would have:

    PatternGenerator -- >  Image -- > FileImage
                             |----- > PatternImage

The essential problem is that each basic chunk of functionality has been unnecessarily split out into new classes even though all the functionality makes more sense together in a single (useful) class. I'm fairly sure my simplification above would easily support everything we already do now (with a little aliasing for legacy support).

Anyway, there is no point continuing as I don't think anyone is going to invest the time to make this file sane. :-(

This shouldn't hold up the merge but I might move this suggestion to a new Issue "Clean-up imagen.image class hierarchy" or similar.

[jlstevens]

I'm sure this is getting messy, but please rebase your fork off of master because I don't want to ever see that horrible code again!

[spiglerg]

Ok I'm closing this PR now. I'll commit the new one as soon as I fix a bad bug. ;)

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

I'm sure this is getting messy, but please rebase your fork off of master because I don't want to ever see that horrible code again!

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-49173476.

[jlstevens]

Ok sure! At this point I think we should merge as soon as everything is working - even if not completely polished, this will be a huge improvement to image.py!

[spiglerg]

Ok!

Right now there is a problem with channels(): it happens that the image has not been generated yet, that's why the call to self(). However, in some cases (like Selector), we don't want to call it again (not to increment the counter?), and in some other cases (I don't understand why, but probably because no params are passed to the function) the image is "regenerated" like the channel_data was the image just loaded, that is, zooming into a smaller portion every time.

I don't think I've explained it well, but it's under way to fixing. ;) (overriding channels() where required)

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

Ok sure! At this point I think we should merge as soon as everything is working - even if not completely polished, this will be a huge improvement to image.py!

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-49185972.

[jbednar]

I'm not sure what you mean; I was arguing that, for safety, channels() should not be used for the same PatternGenerator as __call__() is. So channels should do self(), and it won't be *again", but instead of anyone else calling it. ChannelsGeneratorSheet should then not do __call__(), just channels().

[spiglerg]

Hmm ChannelGeneratorSheet currently calls ".generate()". "channels()" doesn't "call()" by default, it only does when no data is available (eg, a FileImage was created but not called, so both it's _image and _channel_data are empty).

?

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

I'm not sure what you mean; I was arguing that, for safety, channels() should not be used for the same PatternGenerator as call() is. So channels should do self(), and it won't be again", but instead of anyone else calling it. ChannelsGeneratorSheet should then not do *call(), just channels().

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/29#issuecomment-49196160.

[jbednar]

My proposal sent earlier was that users of PatternGenerators now use either __call__() or channels(), but not ever both. So channels() should always do __call__() explicitly or do something that is equivalent to __call__() (but generating channels instead of a single array), and it doesn't need to check whether __call__() has already been run; it's almost certainly too difficult for us to ensure that this would work safely in general, and also not necessary as far as I can see.

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

[spiglerg]

Hmm but if we rely on objects using call we can keep the "average-channel" compatibility (ie, behaving like a normal PG) to do every basic operation, and only access channels() for specific muiltichannel needs.

In the new case we would need to make sure that the generators are not updated in any way other than using channels() (to avoid double generation of patterns, which might for instance change the Selector's index), which might be tricky (eg, super-class code that automatically updates or generates data).

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

My proposal sent earlier was that users of PatternGenerators now use either __call__() or channels(), but not ever both. So channels() should always do __call__() explicitly or do something that is equivalent to __call__() (but generating channels instead of a single array), and it doesn't need to check whether __call__() has already been run; it's almost certainly too difficult for us to ensure that this would work safely in general, and also not necessary as far as I can see.

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/imagen/pull/29#issuecomment-49198902.

[spiglerg]

I've submitted the new pull requests!

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

Hmm but if we rely on objects using call we can keep the "average-channel" compatibility (ie, behaving like a normal PG) to do every basic operation, and only access channels() for specific muiltichannel needs.

In the new case we would need to make sure that the generators are not updated in any way other than using channels() (to avoid double generation of patterns, which might for instance change the Selector's index), which might be tricky (eg, super-class code that automatically updates or generates data).

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

wrote:

My proposal sent earlier was that users of PatternGenerators now use either __call__() or channels(), but not ever both. So channels() should always do __call__() explicitly or do something that is equivalent to __call__() (but generating channels instead of a single array), and it doesn't need to check whether __call__() has already been run; it's almost certainly too difficult for us to ensure that this would work safely in general, and also not necessary as far as I can see.

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/imagen/pull/29#issuecomment-49198902.