Add type to loop variables only #947
Conversation
src/pyscipopt/lp.pxi
Outdated
| @@ -63,6 +63,7 @@ cdef class LP: | |||
| lb -- lower bound (default 0.0) | |||
| ub -- upper bound (default infinity) | |||
| """ | |||
| cdef int i | |||
| nnonz = len(entries) | |||
There was a problem hiding this comment.
This must then also be an int, similar further down.
There was a problem hiding this comment.
I can do this (did so for the examples you pointed out), but I think there's no gain from Cython's POV.
There was a problem hiding this comment.
Depends on how range works. For me it would be just more consistent. At some point it needs to be converted and maybe it is better to do it only once.
| @@ -981,6 +984,7 @@ cdef class Solution: | |||
|
|
|||
| def __getitem__(self, Expr expr): | |||
| # fast track for Variable | |||
| cdef SCIP_Real coeff | |||
There was a problem hiding this comment.
Maybe add a new line after each cdef block.
There was a problem hiding this comment.
Did this, but it does look a bit unnatural in some places
There was a problem hiding this comment.
If the function block is not divided by newlines already, this can also be omitted.
Co-authored-by: DominikKamp <[email protected]>
| @@ -168,6 +168,8 @@ cdef SCIP_RETCODE PyConsFree (SCIP* scip, SCIP_CONSHDLR* conshdlr) noexcept with | |||
| return SCIP_OKAY | |||
|
|
|||
| cdef SCIP_RETCODE PyConsInit (SCIP* scip, SCIP_CONSHDLR* conshdlr, SCIP_CONS** conss, int nconss) noexcept with gil: | |||
| cdef int i | |||
|
|
|||
| PyConshdlr = getPyConshdlr(conshdlr) | |||
| cdef constraints = [] | |||
There was a problem hiding this comment.
Hmm I don't know, actually
src/pyscipopt/scip.pxi
Outdated
| @@ -4880,6 +4920,7 @@ cdef class Model: | |||
| """ | |||
| cdef SCIP_Bool success | |||
| cdef int _nvars | |||
There was a problem hiding this comment.
Why use _nvars instead of nvars here? Why below not? Could this be made consistent as well?
There was a problem hiding this comment.
Would you rather it being all _nvars or all nvars?
There was a problem hiding this comment.
If the scope allows, nvars. At some spots _nvars seems to be not even used afterwards.
There was a problem hiding this comment.
I suppose the same argument goes for _vars.
EDIT: Actually, I guess not, since it seems to be used whenever it's referring to scip variables. The same might have been true for _nvars.
There was a problem hiding this comment.
I think everything should be done now, @DominikKamp
Co-authored-by: DominikKamp <[email protected]>
Similar to #946, but without adding types to the arguments, as it was controversial.
We should probably also add a performance test at some point. Naturally, SCIP should be where the majority of time is spent, but maybe someone is doing a lot of stuff on the Python side.
The performance gain of this is probably marginal, but faster is better than slower, I suppose.