jbednar
commented
11 years ago Looks like a whole lot of work; glad I didn't have to do it! Looks very reasonable to me. I've made small comments in the code itself, but overall:
- Each file needs a docstring or comment at the top explaining what that file is for, and how it fits in.
- What's the rationale for having so much of it implemented in C/C++ rather than in Cython? I thought Cython was supposed to be the way we wanted to do things now, getting away from bare C/C++? This way seems to be leading to duplication of the same ideas (DivisiveNormalizeL1, etc.) at many different levels; is it possible to avoid that by having one implementation in Cython? Or can Cython not talk to external C libraries well? Even if it can't, can the C/C++ portion be limited to the eigen interface only? There's lots of other stuff in there right now, and I don't know if it's necessary. But if it is, then explain why in each files's docstring or comment headers.
- We need to add tests, especially ones comparing the sparse and dense implementations. Having a dense equivalent of the sprouting and retraction code (as mentioned in the diff comments) would be very helpful here, as otherwise we won't be able to do that. In any case we need to capture the results from the current code and store them as a reference, so that we can detect changes later.
philippjfr
Proposed Topographica sparse support for using Eigen3. Eigen3 auto-mirror added as submodule in external directory. Uses C++ and Cython wrappers to interface Eigen3 with SparseCFProjection and all files are in topo/sparse/. SparseConnectionField objects are views of the sparse matrix owned by the SparseCFProjection and support the normal interface for CFs.
Code has been tested for numeric accuracy and matches dense results identically if active_units_mask is disabled, otherwise matches to 7 dp. Supports sufficient functionality for real work but code may require further tidying.
Philipp & Jean-Luc