coin-or / python-mip

Python-MIP: collection of Python tools for the modeling and solution of Mixed-Integer Linear programs
Eclipse Public License 2.0
525 stars 92 forks source link

added an easy way to use variables in numpy tensors and some optimizations #100

Closed bonelli closed 4 years ago

bonelli commented 4 years ago

This is the first part of my changes, here I just introduced the tensorial notation with numpy and a basic test for it which measures time of model preparation.

The test is meant to be able to measure speed improvements in model creation. On my laptop the test runs in approx. 22 seconds

bonelli commented 4 years ago

Travis found some issues, I'll push fixes

bonelli commented 4 years ago

@h-g-s I've added everything except for the link

include a link to a description of tensor (is this one good ? https://www.kdnuggets.com/2018/05/wtf-tensor.html )

that is a nice link, where should I include it?

As for the example, I choose a basic example and showcase how matrix notation allows you to get rid of most for loops, even when using many variables. I think that's all there is to it for any engineer or applied-math guy.

bonelli commented 4 years ago

I'm pretty much finished adding all tensorial features I want for my project. Main features:

h-g-s commented 4 years ago

It is looking great @bonelli ! Just some additional small changes:

Also, this cosmetic change, reformat some files with black and line width 89 to preserve our default style: (automate test) 3.40s$ if [[ $TRAVIS_PYTHON_VERSION != 3.5 ]]; then python3 -m black mip --line-length=89 callbacks.py entities.py lists.py ndarray.py model.py

bonelli commented 4 years ago

@h-g-s

Also, this cosmetic change, reformat some files with black and line width 89 to preserve our default style: (automate test) 3.40s$ if [[ $TRAVIS_PYTHON_VERSION != 3.5 ]]; then python3 -m black mip --line-length=89 callbacks.py entities.py lists.py ndarray.py model.py

I reformatted with black line-length 89 all files with newly added code and all files you listed, not sure why you asked me to format "callbacks.py" and "lists.py" though. They were not part of the PR in the first place, and this messes up commit logs a bit, nothing dramatic.

Everything has been pushed, waiting for new revision @h-g-s and @jurasofish

h-g-s commented 4 years ago

Hi @bonelli , thanks. It seems that tests passed for python 3.5, 3.7 and 3.8 but not for 3.6, could you check ? https://travis-ci.org/github/coin-or/python-mip/jobs/688984604

bonelli commented 4 years ago

Hi @bonelli , thanks. It seems that tests passed for python 3.5, 3.7 and 3.8 but not for 3.6, could you check ?

All hail the mighty CI/CD! Travis discovered that I put the wrong dependency version for numpy, the features I'm using are there since 1.17 and not 1.16 as I believed.

Fixed now, Travis is green

h-g-s commented 4 years ago

Hi @bonelli , just merged to master, just made some small changes to make the sphinx autodoc documentation work. One thing, could you add a docstring documentation to the class LinExprTensor ?

bonelli commented 4 years ago

Hi @bonelli , just merged to master, just made some small changes to make the sphinx autodoc documentation work. One thing, could you add a docstring documentation to the class LinExprTensor ?

Will do. Do you prefer a new branch and PR or this same one?

h-g-s commented 4 years ago

another thing, in some functions, such as _iadd_tensor_element could have types too, for example,

it seems that element is one of the tipes: mip.LinExpr, mip.CutPool

then, the type of element can be declared as:

element: Union[mip.LinExpr, mip.CutPool]

h-g-s commented 4 years ago

Hi @bonelli , just merged to master, just made some small changes to make the sphinx autodoc documentation work. One thing, could you add a docstring documentation to the class LinExprTensor ?

Will do. Do you prefer a new branch and PR or this same one?

added another comment, well, you could update your master with my small changes and then make a pull request to master

tuliotoffolo commented 4 years ago

Hello @bonelli. First of all: thank you very much for your nice contribution.

I've been a bit away and unfortunately lost the discussion. I'm back now and wanted to add something relevant to the discussion. In the master's branch current version, numpy is a dependency. Although I understand that numpy is widely used -- I always use it myself --, it seems unreasonable to force all our users to have it installed. Not to mention that in the past numpy was not really supported by PyPy. Therefore, I plan adding some 'try/except' in model.py and in ndarray.py, so that numpy is no longer a dependency.

What do you think? Any alternative suggestion?

bonelli commented 4 years ago

In the master's branch current version, numpy is a dependency. Although I understand that numpy is widely used -- I always use it myself --, it seems unreasonable to force all our users to have it installed. Not to mention that in the past numpy was not really supported by PyPy. Therefore, I plan adding some 'try/except' in model.py and in ndarray.py, so that numpy is no longer a dependency.

What do you think? Any alternative suggestion?

I don't see any problem in having numpy as an optional dependency, but I do like the idea that in normal circumstances it gets installed automatically when I type pip install mip, and with the right version too. In most normal cases having numpy as a dependency is no big deal anyway.

Maybe put try/except around relevant portions and use extras_require into setup.py to have it installed with the optional? This way if one really wants a thin virtualenv, python-mip can run without, but usually it gets installed with numpy.

bonelli commented 4 years ago

it seems that element is one of the tipes: mip.LinExpr, mip.CutPool

Actually it could be real numbers and boolean too, and it will often be so if the problem is sparse. When a sparse problem achieve a tensor with LinExpr in some elements and zeros in most other elements, the comparison with a constraint rhs will give booleans in those elements, and if the problem is well posed, all those booleans are True and those constraints are verified by construction.

At the moment I simply ignore reals and booleans, because they're not part of the subsequent optimization problem, but now that you make me think about it we could check that at least they are all True before ignoring them.

bonelli commented 4 years ago

Hi @bonelli , just merged to master, just made some small changes to make the sphinx autodoc documentation work. One thing, could you add a docstring documentation to the class LinExprTensor ?

Will do. Do you prefer a new branch and PR or this same one?

added another comment, well, you could update your master with my small changes and then make a pull request to master

added pull request #104