add type hints (Experimental) #755
Conversation
|
|
There was a problem hiding this comment.
sorry for the delay.
I've checked around half.
Many things look good. Many others require careful investigation. I'm particularly skeptic on the large Union types you have for some of the other arguments.
In some places you're changing the behavior of the code (True instead of 0)
You use casting a lot. I'm not sure if it's really required if we're correctly defining the types. Also, there are some ignore that I believe should not be needed with the correct typing.
You're also cleaning a lot of compatibility code (which is fine) but I'm not sure if that's breaking something. Could you at least comment on the pieces of code you're taking out?
| ) -> Tuple[ | ||
| int, | ||
| int, | ||
| int, | ||
| int, | ||
| int, | ||
| "Array[c_double]", | ||
| c_double, | ||
| "Array[c_double]", | ||
| "Array[c_double]", | ||
| "Array[c_char]", | ||
| Any, | ||
| Any, | ||
| Any, | ||
| Any, | ||
| "Array[c_double]", | ||
| "Array[c_double]", | ||
| "Array[c_double]", | ||
| "Array[c_char_p]", | ||
| "Array[c_char_p]", | ||
| "Array[c_char]", | ||
| Dict[int, "LpVariable"], | ||
| Dict[Any, Any], | ||
| ]: |
There was a problem hiding this comment.
can you define this type outside and then use it here? it's too big to be readable
There was a problem hiding this comment.
I think ok. we can define a type alias in other file and import to use at this.
| """ | ||
| uses existing problem information and solves the problem | ||
| If it is not implemented in the solver | ||
| just solve again | ||
| """ | ||
| self.actualSolve(lp, **kwargs) | ||
|
|
||
| def copy(self): | ||
| def copy(self) -> "Self": |
There was a problem hiding this comment.
Self works here? shouldn't we put LpSolver?
| "LpAffineExpression", "LpVariable", "LpConstraintVar", int, float, None | ||
| ], | ||
| ) -> "LpAffineExpression": | ||
| self = cast(Union[LpVariable, LpConstraintVar], self) |
There was a problem hiding this comment.
why is the cast necessary? doesn't LpAffineExpression accept LpElement?
| other: Union[ | ||
| "LpAffineExpression", | ||
| Dict[LpVariable, Union[int, float]], | ||
| List[ | ||
| Union["LpAffineExpression", LpVariable, "LpConstraintVar", int, float] | ||
| ], | ||
| Iterable[LpVariable], | ||
| LpVariable, | ||
| "LpConstraintVar", | ||
| int, | ||
| float, | ||
| None, | ||
| ], | ||
| ) -> "LpAffineExpression": |
There was a problem hiding this comment.
this other is repeated in several places. (1) I'm not sure it's correct (2) if it is, we should create it once and re-use it
| self.constraints = _DICT_TYPE() | ||
| self.constraints = dict() |
There was a problem hiding this comment.
why the change? it's the same I believe
| self.noOverlap = 1 | ||
| self.noOverlap = False |
There was a problem hiding this comment.
this changes the code. Shouldn't it be True?
There was a problem hiding this comment.
Sorry, it should be True.
| self.lastUnused = 0 | ||
| self.lastUnused = True |
|
|
||
| # constraints | ||
| # we change the names for the objects: | ||
| def edit_const(const): | ||
| const = dict(const) |
There was a problem hiding this comment.
why are you taking out the shallow copy?
|
Thanks for your review. At first, I think I can typed this lib suitably. But after some research, I'm got a discouraged.
Sorry, the workload for this task far exceeds my estimation. In contrast, the benefits obtained from typed are too low. Now I no longer have courage and energy to modify these codes. You can close this pr. |
|
Thank you for your efforts though! With your permission I will branch your code and I will try to make it compatible. I understand the pains of dealing with legacy code. I was not in this project at the time and it's definitely a challenge also for me. I'm confident that I can make something useful with the work you have. Thanks again for the help! |
#650
This pr still incomplete, some functions are hard to type, like
LpVariable.matrixandLpVariable.dicts.And lots of usage of
castand# type: ignoreto adapt old code base (some replace by not None assert).