search

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

Reintroduction of in-place-operators for LinExpr #108

Closed sebheger closed 2 years ago

sebheger commented 4 years ago

With release 1.9.0 (in comparison to 1.8.2) the in-place-operators of LinExpr have been set to "Deprecated". I would suggest to reintroduce it again. We have introduced CBC to our company optimization platform (besides a commercial optimizer) a few weeks ago and would be glad if interfaces keep as stable as possible. I am wondering why this nice and convenient feature (which APIs of commercial solvers also provide) has been removed within the changes for #100.

jurasofish commented 4 years ago

To answer your question directly, in-place mutation was removed to allow performance improvements when multiplying by 1 or 0 in #98.

In a more general sense, I think two things this library could do to really strongly improve quality and consistency are:

Semantic versioning would have at least mitigated this issue as it would have pushed the version to 2.x.x (as I said https://github.com/coin-or/python-mip/issues/98#issuecomment-627655070). I think users (such as you @sebheger) are assuming that this library follows semantic versioning. That's a reasonable assumption - I'd make the same - but it's not the case.

I really like this library, which is why I'm helping out a little with development, but the development process is very cavalier. I think it's really important to get this sort of stuff nailed down considering the growing public use of this library (which isn't surprising, it's a great library).

sebheger commented 4 years ago

In general, I totally agree with what @jurasofish said. Additionally, I would suggest to not deliver "dirty versions" of CBC within the release. You could incrementally deliver a new, official version with a minor update (1.9.x) corresponding to the right semantic versioning.

In this case, regarding the deprecated function, I would suggest adding the functionality back. Based on a first look into 3772f068d76638622b30972b14cbc10dfb5e102d there will be no need to remove it. As I noticed from the discussion in #98, the in-place operation is slow. But in my option, this is no cause to remove it ;-)

tommyod commented 4 years ago

I think it's really important to get this sort of stuff nailed down considering the growing public use of this library (which isn't surprising, it's a great library).

Agreed. Also, I think many talented developers that might be interested in contributing simply move on when they see that the development process does not follow standard practice. This creates a negative feedback loop.

tuliotoffolo commented 4 years ago

Hello @sebheger and @jurasofish, I was away and did not follow #98 and #100 at all. I only interfered in the very end when I noticed that numpy was added as a dependency (although we don't need to do so currently).

After your message I read all code changes. I should tell you that I myself use in-place operators very often... Anyway, @jurasofish is right: we must and will improve the development process.

Concerning in-place operators: I agree that they represent a very useful functionality and should be added back.

h-g-s commented 4 years ago

Hello everyone,

Well, If in-place operators are really useful for building models (personally I don't use them) then there should exist some examples and tests using them.

With enough tests we can keep a rapid release cycles without the risk of breaking important things.

jurasofish commented 4 years ago

Hello everyone,

Well, If in-place operators are really useful for building models (personally I don't use them) then there should exist some examples and tests using them.

With enough tests we can keep a rapid release cycles without the risk of breaking important things.

Agreed, features should have tests

tuliotoffolo commented 4 years ago

Yes, definitely! I will add some models implemented using this feature. Em 26 de mai de 2020 19:55 -0300, Michael Jurasovic notifications@github.com, escreveu:

Hello everyone, Well, If in-place operators are really useful for building models (personally I don't use them) then there should exist some examples and tests using them. With enough tests we can keep a rapid release cycles without the risk of breaking important things. Agreed, features should have tests — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

sebheger commented 4 years ago

@h-g-s As requested I have found an example where somebody uses in-place operators to build an expression. Honestly, there is also an add function available to extend the LinExpr with the content of another expr. But here a combination of in-place and sum-iteration is used, for me it is clear and readable. I see no urgent need to refactor this piece of code ...

demand_expr = LinExpr()
demand_expr += supply_demand_data.loc[group, 'Demand_{}'.format(time)]
demand_expr += sum(demand_shift_plus[(group, time)])
demand_expr -= sum(demand_shift_minus[(group, time)])
tuliotoffolo commented 4 years ago

Excellent! I agree that it is readable as it is.

Em ter., 30 de jun. de 2020 às 09:41, Sebastian Heger < notifications@github.com> escreveu:

@h-g-s https://github.com/h-g-s As requested I have found an example where somebody uses in-place operators to build an expression. Honestly, there is also an add function available to extend the LinExpr with the content of another expr. But here a combination of in-place and sum-iteration is used, for me it is clear and readable. So I see no urgent need to refactor this piece of code ...

demand_expr = LinExpr()demand_expr += supply_demanddata.loc[group, 'Demand{}'.format(time)]demand_expr += sum(demand_shift_plus[(group, time)])demand_expr -= sum(demand_shift_minus[(group, time)])

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/coin-or/python-mip/issues/108#issuecomment-651765548, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEQHYIN44QVP3BMSXCXMADRZHMPTANCNFSM4NKEYHSA .

tuliotoffolo commented 3 years ago

Dear all, After some productive discussions with Haroldo, we would like to debate this subject again and close this issue.

I am in favor of reintroducing the in-place operations for LinExpr, but would like to hear arguments from those who are against the reintroduction. I do believe we are losing too much for a rather limited gain (improvements when multiplying by 1 or 0).