Sparsematrix removal 2 #169
Conversation
breznak
added
API_BREAKING_CHANGE
code
only user SpatialPooler was transformed to use Connections in #93, now SparseBinaryMatrix is unused. Removing c++ code, bindings, tests
the .hpp is all commented out, .cpp is used
used as optimization for python set, now built-in python set() is used. This file has long been deprecated.
unused since PR #170 - SDR Classifier uses 2d map map<int, map<int, double>>
this is all of the custom Sparse*Matrix classes removed,
|
This should be reviewed after #170 is merged. |
|
@dkeeney @ctrl-z-9000-times Please review, this completes the *Matrix cleanup, all this code has been old and unmaintaned, now is unused and removed. That makes our codebase some 40k LOC lighter.
This was not a public facing API, so there are no worries in that regard. |
Let's continue this discussion in #216
hmm, toggling fPIC based on if bindings are requested does sound as a good solution, on the other hand, is keeping fPIC and linking as a shared lib too much of a problem? |
I think your approach is good. Use -fpic for python bindings and no -fpic otherwise. Bonus points for an override flag allowing savvy users to control this themselves. |
failing test on OSX CI |
|
@ctrl-z-9000-times @dkeeney this PR is updated and passing again, ready for next round of reviews |
There was a problem hiding this comment.
@Thanh-Binh is right it breaks functionality in Py repo.
I'm not going to approve this PR until we all agree on a plan for dealing with the broken python repo. We don't need to do the plan, just having one is enough for now.
- We all have different ideas on what to do about the python repo, and no one wants to touch it because it's as messy as this repo was at the start.
- The python repo is the point-of-entry for all new users. Also everyone prefers using python over C++ until performance becomes a problem.
- We have no continuous integration for the things which this PR would break, which is a problem in itself but not a blocker for this PR. This PR will not be the first to potentially break the python repo, if it hasn't already been broken by us without our knowledge.
|
Now that we have a plan for dealing with the broken python repo, this PR can proceed. Before final approval we need a written plan describing:
Maybe leave these notes in issue #216, or open a new issues as needed? |
|
Thanks, I'll work on this later the week. FYI, these are total files removed in this PR:
I'll ignore the bindings, as that repo is not synced to our new bindings anyway.
These are the focus to check
I'll ignore the tests as they match the removed main classes. |
Conflicts: bindings/py/cpp_src/bindings/math/py_SparseBinaryMatrix.cpp bindings/py/cpp_src/bindings/math/py_SparseMatrix.cpp bindings/py/cpp_src/bindings/math/py_SparseTensor.cpp src/nupic/math/Math.hpp src/nupic/math/SparseBinaryMatrix.hpp src/test/unit/algorithms/Cells4Test.cpp
ctrl-z-9000-times
force-pushed
the
sparsematrix_removal_2
branch
from
a5ff516
to
8c5d36f
Compare
Removes all of math/Sparse*Matrix codes, which are not used;
this code should be replaced with Connections (as in SP #93 ) or Eigen (sparse)Matrix.
this provides vast modernization of the codebase (no code with ifdefs, hacks, etc) making it much easier to port.
Removes dependency on Boost, so we can get rid of it
Fixes #104
Allowed after #93
Follow-up to #149
Fixes #103
Fixes: #213