New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the add_constraint method from MixedIntegerLinearProgram #7311
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:5
Jeffrey Kantor smartly noticed it may not be a good idea to have > be an alias for >= when dealing with integer programs... Fixed :-) Nathann |
comment:8
I started looking at your patch. I think it shouldn't be too hard to review. All tests pass ! However, I have one question: is there a reason why you implement all operators <, >, >=, etc. ? You could only implement Another strange thing is that the tests pass only when I install GLPK. With CBC, nothing works. Is that normal ? |
comment:9
Hello !!!! I remember having tried the two This patch has had many different versions At some point some of these classes were "cdef class", then got back to usual python... I remember having noticed at some point that if I were to define < > == ... using richcmp instead of the usual python operators, other low-level comparators would be used by default instead of mine. Well, I had many troubles with this class, so perhaps I should give it a try again now that it works again :-) It is not normal at all that nothing works with CBC :-/ Thank youuuuuuuuu !! Nathann |
comment:10
Here is what I did:
If I'm not mistaken and if my installation is not broken, it means that the examples you included in the docstring are based on GLPK and not CBC. |
comment:11
Then it is not a problem at all :-) The errors you see are in no way related to the present patch. You can see in all of your error messages that the line at fault if followed by : optional - requires GLPK. Note that there is no mention of CBC in those. These lines are GLPK-specific as they are those who define the functions exporting a LP to a .mps or a .lp file, which make use of the GLPK library. You had me worried for a while ! :-) Nathann |
comment:12
Great ! Sorry for my comments, I'm not familiar enough with these two optional packages. I'll resume reviewing the patch later today. |
comment:13
I looked once again at your patch. It seems correct, but I have a few comments:
Woudn't be better to access this value by a function that LinearFunction provides ? More precisely, LinearFunction objects should have a method
Short of that, the code seems ok, and all tests even optional pass. There are some typos in the doc, but I can fix it while reviewing. As soon as you answer/correct item 1, I should be able to finish reviewing it. |
comment:14
Here it is ! A new .dict() method for LinearFunction :-) I looked for long lines in the code, but I did not know how to fold them without making it much harder to understand... :-/ If you find one you think can be safely cut, do not hesitate though :-) Thank you again for your help ! Nathann |
comment:15
Hello again Nathann ! I looked once again at your patch. You'll see that I added one on top of yours: it only modifies the formatting in the code and in the documentation, nothing major. In particular, I removed empty lines where they didn't seem necessary to me. I should give a positive review to your ticket very soon, but before that, there are some questions I would like you to answer.
Sorry again for bothering you ! Don't forget to apply my patch if you want to add some modifications to yours. Next time should be a positive review ! |
comment:16
It's true that you do not modify the |
comment:17
Hello !! I will be answering your questions today :-) Meanhile, I understand that I access many fields without calling methods, but in the end as this class is very short (and as I don't directly access fields without calling methods when outside of the definition of this class), I feel like adding many other methods would just create more calls to function, thus slowing it a bit, without really improving the readability. Well, I don't mind changing it anyway, but ... What do you think ? Nathann |
comment:18
I agree with you, since speed is very important in MILPs, it is reasonable... Forget about what I said. However, I still think there is a reasonable way to extract the constant coefficient without having to modify the object returned by the |
comment:19
Hello !!!! First, this new patch contains first my file trac_7311.patch, along with your corrections from trac_7311_review-abm.patch (and my thanks for your time!). # (we talked about this together, but for the completeness of this ticket...). The names for Variables and Constraints are totally "useless" at the moment. Well, they can be useful only when one is trying to export a MixedIntegerLinearProgram object to a .mps or a .lp file using the methods write_mps and write_lp. This way, anyone can export the program and try to solve it using a different solver, as these files are standard types to encode a LP, and clearly having names in such a file instead of having variables like x1 x2 x3, .... and constraints like c1 c2 c3, ... can help at some point :-) Besides, I have not lost hope of touching the .show() method so that it would use the names of these variables/constraints when the user wants to see his LP. # It is dangerous. And it is a mistake :-) # I thought the documentation from And here is the new version of the patch :-) Nathann |
Attachment: trac_7311.patch.gz Minor doc and code formatting fixes -- apply on top of Nathann's patch |
Reviewer: Alexandre Blondin Massé |
comment:20
Attachment: trac_7311_review-abm.patch.gz I tested Nathann's patch on sage-4.3.3. All tests passed and the documentation builds fine, except for the signature of many functions since the main involved file is a Cython file (this issue seems to have been solved in #8244). Note that all non optional tests passed with neither GLPK nor CBC installed, and all tests including optional ones pass when I installed both GLPK and CBC. I made some minor changes on code formatting and documentation. If Nathann agrees with my changes, I allow him to set this ticket to a positive review. |
Author: Nathann Cohen |
comment:21
Thank you very much !! But what do you mean about problems with the signatures of functions ? O_o Nathann |
comment:22
When you run
Sphinx has some problem to deal with Cython files when generating the documentation and this issue was discussed in #8244. |
comment:23
OOps, ok, I see.... Then I accept your positive review and your fixes :-) Thank you very much !!!! Nathann |
comment:24
While retesting the patch, I noticed that some tests didn't pass when one doesn't have CBC or GLPK installed. This means that some doctest wasn't tagged "optional" properly. Sorry about that, Nathann, but there is still some work to do. I'll try to identify the bugs tonight. |
comment:25
Sorry about my last comment, everything seems fine. This is very complicated to test optional packages properly... I have too many copies of sage on my computer. Positive review. |
comment:27
Merged into 4.4.alpha0:
|
Merged: sage-4.4.alpha0 |
The min and max arguments should accept variables too, and not just real values or None. This is now possible, along with more complex expressions inside of add_constraint and without even using min/max ( which are still the most efficient way to define constraints )
Also fixes an important bug (copy was used instead of deepcopy, which produced wrong results)
CC: @sagetrac-abmasse
Component: numerical
Author: Nathann Cohen
Reviewer: Alexandre Blondin Massé
Merged: sage-4.4.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/7311
The text was updated successfully, but these errors were encountered: