Assoc mem v2

[xchoo]

• Changed output function from step function to filtered step function (smoother output)
• Used ClippedExpDist for default intercepts distribution (for better filtered step function approximation)
• Increased default number of neurons used in the AssociativeMemory
• Split up code that does wta, output thresholding, and default output vector logic. This enables users to specify different contexts for each of these operations.
• Moved associative memory test code to the proper place (networks/tests rather than in spa/tests)
• Updated test code for both network version and spa version of the associative memory.

Note: Contains both assoc_mem_reorg and expdist branches.

[jgosmann]

This parameter does not appear in the argument list.

[jgosmann]

I looked over this. The two major issues:

• The 1.5 constant issue (see comments above). I don't think we need the possibility to adjust this as this can be done with the incoming connection. However, the 1.5 seems pretty magic and I wonder whether it is better to use 1.0 as a default. This might need require all inhibiting connection to have a transform of 1.5, but at least there is no magic number multiplying the inhibition which is not obvious when not looking into the source code.
• There seems some code duplication in __init__ from add_input and add_output. Is there any reason not to use add_input and add_output in __init__?

Also added a few minor comments. I could probably fix most of this myself if we agree on above issues.

Would be nice to get this merged soon for other PRs related to the associative memory.

[tbekolay]

I'm using these for a thesis model so I figured I'd use the most recent version. I cleaned up the code and history a bit and removed the code duplication that @jgosmann mentioned; see my assoc_mem_v2.1 branch.

I find the add_input and add_output functions quite confusing. From their names, I assumed that they would add new ensembles to the network, to accommodate for additional input and output vectors that perhaps weren't anticipated when the memory was initially created. This isn't actually what they do though, they add new input sources and output sources; the input source associates a completely separate set of input vectors with the output vectors that are already in the associative memory. Even if I allow myself to think that this use case exists, it's rather confusing to me, as the mapping from the new input vectors to the existing output vectors is implicit; I added some input, but I haven't told you what output they should map to. It just uses the output in the order that I originally provided them, which makes sense once I know it, but isn't clear from any of the docstrings or typical use cases. For example, if you're using this memory as an autoassociative memory, and then you give it a new input with new vectors, you have turned this into a heteroassociative memory. Perhaps you didn't realize that this was happening though, and assumed that it would create a corresponding set of outputs as well (the output_vectors argument was automatically filled in in the constructor, after all).

If I happen to realize this and then add a new output, then I've actually created a weird set of interactions such that the both sets of inputs are associated with both sets of outputs. Again, I see the usefulness of this, but I think that the API does not give the correct mental model of what's going on inside the associative memory. When you create it, it sets up an immutable mapping from input vectors to output vectors -- yet, we allow you to add new inputs and outputs. In actuality, what you're adding aren't new inputs and output, but new associations between a new input source and existing outputs, and new associations between existing inputs and a new output source. I need to think more to come up with a more concrete way to express this, but I think we're missing some explanatory abstraction here.

[xchoo]

Oh... Yeah. The add_input and add_output functions were added to cater to very niche cases (in my models). I use the add_input function only once in Spaun, and it's to add a bias to the input vector of an AM. And the add_output function is used to create multiple output mappings -- e.g. if you have a concept->vis_sp map, and you want to add a concept->mtr_sp map using the same cleanup on the concept vector. They were never intended to be used together.

They could be renamed to add_input_vector_set and add_output_vector_set (or better documented).

@jgosmann Regarding the 1.5 inhibitory scale, it is possible to change the scale (well, effective scale). All you have to do is put a scale when you connect to the inhib node. The reason for the 1.5 scale is: if you have a threshold of 0, you'd need at least an inhibitory input of 1 to shut off the ensemble. And I added a 50% increase to the scale for a buffer. (Same thing for a normal inhibitory connection, you need to have at least 2 to inhibit an ensemble, but by default I use 3 as the inhibitory scale). Theoretically, what you need is an inhibitory scale of (1 - threshold) * 1.5.

[jgosmann]

I know, why the 1.5 inhibitory scaling is necessary. What I am worried about is that if someone wants a specific scaling they have to either look into the source code or do it by trial and error. Also, it seems plausible to assume that the inhibitory connection has a scale of 1.0 like all unscaled connections. But it requires the user to be aware that they have to add the 1.5 scale on their connections which might be forgotton? I could also see an argument to use a scaling constant (i.e. 1.5) to just make it work without further scaling supplied by the user. But I feel in that case the scaling should be better documented.

[tbekolay]

After some chatting with @xchoo, I've moved the assoc_mem_v2.1 branch here. Mostly, it's just moving some parts around to hopefully make things more clear. See bb43904 and ed0f07a for the diffs. I haven't addressed the 1.5 scaling or the add_input / add_output function names.

[Seanny123]

@jgosmann are you okay with keeping the function names in light of the new documentation?

How do people feel about adding the inhibition_scale as a parameter? So if you don't want the associative memory to be inhibitable, you just set it to 0, but otherwise it's defaulted to 1.5? Alternatively, if everyone involved in this discussion is willing to put their foot down, I'm fine with documenting the value of 1.5.

After that's been decided I can update @ikajic's examples for the new code.

I'd like to get this code into a state where it can be merged as need it for a model I'm building. Since that makes a total of three people using it regularly, I think that's a good reason for it to be merged.

[xchoo]

I'm not a fan of having the 'set inhib_scale to zero to disable inhibition' idea. Mainly because you then have to know what value to set the weight at if you want inhibition. As well, if you don't explicitly disable inhibition, the code will create extra connections that are then unused.

[Seanny123]

As an aside, the labels for the spa.Associative memory ensembles should be set more intutively.

[jgosmann]

@jgosmann are you okay with keeping the function names in light of the new documentation?

Can you remind me about which function names we are speaking?

How do people feel about adding the inhibition_scale as a parameter?

Don't like it. It is more an issue of what should the default value be and documentation.

[Seanny123]

@jgosmann the function names are add_input and add_output

I'm with Jan's original proposition that the default value should be 1.0 and that if someone isn't seeing the inhibition they want they up the gain. I consider this a nicer scenario then seeing the inhibition they don't want and having to dig through the source code to figure out where the gain on the connection is being mucked with.

[jgosmann]

Not sure if I am missing any more recent documentation changes, but to me neither the function names nor the documentation make clear to me what exactly the functions do. Would add_input_mapping and add_output_mapping maybe be better names? (But some clarification in the documentation in addition to that would be good too.)

[xchoo]

I'm not sure what you mean by 'seeing inhibition they don't want'. If they don't want inhibition, then they should not enable the inhibitory flag. Otherwise, there will be inhibition included. And if they do see inhibition, it doesn't matter if the initial gain is set to 1, or to 1.5. In either case, the user has to look into the code to see what that value is. (note that they don't need to know the exact value. If they aren't seeing enough inhibition, simply cranking up the weight of the transform to the connection to the inhibit input will do it).

[Seanny123]

@jgosmann I like those names and will edit the code.

@xchoo nvm what I said before, I had the misconception that a too-low gain was easier to compensate for than a too-high gain. I'm now indifferent to the 1.5 constant and am fine with just documenting it.

Jan, in terms of better documentation of the constant, is a comment on the inhibition parameter and an inline code comment acceptable?

[Seanny123]

I added the fixes and tried to rebase to master. Somehow I ended up attributing everything to Jan... How do I got about fixing this attribution? Alternatively, are people okay with merging to master with this mis-attribution?

[xchoo]

It's not a matter of me being fine or not with the inhibition gain being 1. There is a good reason why it is set to 1.5, and i've stated it here: #797 (comment)

I'm not entirely sure what the hangup on the scale is... If I were to define the same parameter for say the ensemble array, the initial value for the inhibitory scale I would have set to 3. Saying that the user would be expecting a 1 doesn't mean anything because if you set it to 1, the inhibition just doesn't work. Same thing here.

If by 'setting the initial inhibitory gain to 1' you mean that to the user facing world, the value is 1, but within the code, it scales it up by 1.5 (or whatever value we choose), then sure, i'm okay with that.

[Seanny123]

I think I misread that comment, which is why I asked if "it was fine" which was a bit of a silly way of asking. Anyways, we've decided to keep with the 1.5.

Does the rest of the code look okay to you Xuan? I just changed the names of add_input and add_output and added more to the nengo.networks.assoc_mem.AssociativeMemory docstring. If you're okay with those changes and the attribution mess-up, are you okay with merging this into Master?

[tbekolay]

Is there a particular need to speed this one up? I am not ok with the authorship change; I have a local version of this branch that I can push to restore it, or we can just attribute it all to Xuan.

[jgosmann]

I would still change the documentation of add_input_mapping/add_output_mapping to better explain what those functions do.

[xchoo]

Yup. The rest of the code looks fine. Although, I'd make the inhibitory gain a class constant (so people will question the magical 1.5 in different parts of the code).

[Seanny123]

Sorry @tbekolay, didn't mean to sound like I was rushing this. I'm fine with attributing everything to Xuan. I'll just do a little Googling to figure out how to do this.

@jgosmann I improved the doctstrings. Do they make more sense now?

@xchoo I've made the change.

[jgosmann]

Docstring for add_input_mapping is grammatically incorrect.

[xchoo]

@Seanny123 You'll have to update the rest of the inhibition code (everywhere it says if self.inibitory) to use that constant as well.

[Seanny123]

@jgosmann accidentally forgot to save before committing. Fixed. Does it make sense now?

@xchoo made other changes. Is it good now?

[xchoo]

looks good

[jgosmann]

Does it make sense now?

yeah

[Seanny123]

I cleaned up the last few commit and fixed the attribution. Xuan, do you think this is ready to merge now?

[xchoo]

yeah. looks good to me

[tbekolay]

The commit message still says ClippedExpDist when it's been renamed to Exponential, but aside from that looks good to me too. I'll merge tomorrow afternoon unless someone beats me to it.