PythonOptimizers / cysparse

Python/Cython library to replace PySparse
http://PythonOptimizers.github.io/cysparse
7 stars 3 forks source link

Trimming Cysparse code base #243

Open lambe opened 7 years ago

lambe commented 7 years ago

This issue came out of discussions with @dpo and continued development of the NLP.py project. In that project, we're supporting several sparse matrix types and it's becoming a problem to carry a lot of different classes around. For example, when converting an NLPModel to a SlackModel, each supported sparse matrix type has its own variation of the SlackModel to handle the different method calls. Another important example is developing sparse QP solvers, because we don't want to have different solvers for each sparse matrix type. We think Cysparse should be the default sparse matrix package moving forward, so we are going to begin studying that.

At the same time, we would like to get cysparse back under active development so that the projects can support each other. Since the main class used in cysparse (under NLP.py) is the int64-float64 combination, we think this should be the focus of development, and set aside the other types. This would also allow us to trim the code base substantially and avoid the reliance on a templating engine.

Obviously a lot of work has gone into supporting many different type combinations, so we want to open this up for discussion. What do @syarra and @counterclocker think especially?

syarra commented 7 years ago

We both, @counterclocker and I, think that the support to all the currently supported types should not be dropped. Not only because we put a lot of efforts to make this possible but also because one of the main goals of the this project was to create a package that would be able to support them all.

Furthermore I am currently using CySparse in another project in which I need huge matrices of long long integers.

CySparse's code base is huge in size due to all the special cases we have to deal with and the fact that Cython doesn't provide a mechanism to deal with different types. If we restrict ourselves to int, uint, float and complex of 32, 64 and 128, it would probably be sufficient.

I think it is a good idea to have a more flexible Python matrix type within CySparse. Something like a factory method that would return the right type of object.

@lambe, to answer your question about the bug in CySparse, I will have a look at the code.

In NLP.py, a lot of different types of matrices are supported. This is nice for modeling as it can bring more users. For the optimization algorithms, this can be difficult to support. I was thinking of creating a matrix super class that will do all the tasks like creating new matrices in the desirable format, update parts of the matrix, ... It will allow us to design matrix independently from algorithms.

lambe commented 7 years ago

I understand why you want to support so many types. Another idea would be to generate all the Cython and C code when the package is installed, rather than downloading it all from Github. That way, we only store the templates in the repo and the package the user downloads is much smaller.

syarra commented 7 years ago

This was intended. It was easier to put all C and generated Cython code in the repo so that those who don't have Cython installed nor Cygenja will not need to install them. They only need a C compiler to install the package.

For other packages we developed, the master branch contains all denerated C and Cython and the develop branch the real source code. We could do the same here. What do you think?

dpo commented 7 years ago

We should.