Skip to content

Annotate several Model methods in the stubs#1220

Open
jonathanberthias wants to merge 9 commits into
scipopt:masterfrom
jonathanberthias:model-annotations-1
Open

Annotate several Model methods in the stubs#1220
jonathanberthias wants to merge 9 commits into
scipopt:masterfrom
jonathanberthias:model-annotations-1

Conversation

@jonathanberthias
Copy link
Copy Markdown
Contributor

This is a first batch of annotations and default parameter values. I believe these are rather straightforward.

I ignored some annotations on purpose: anything related to ExprCons because the annotations for Expr operations aren't correct yet, and anything related to the PY_SCIP_* enums. These can be revisited at a later stage.

Part of #1072

@Joao-Dionisio
Copy link
Copy Markdown
Member

Awesome @jonathanberthias ! Can you also adapt the stub checker for this, to force new contributions to annotate as well?

@jonathanberthias
Copy link
Copy Markdown
Contributor Author

Awesome @jonathanberthias ! Can you also adapt the stub checker for this, to force new contributions to annotate as well?

I'm not sure that's possible yet, I only annotated a tiny part of the API so we would need to "finish" everything before we can forbid broad annotations like ˋIncompleteˋ.

BTW the automated script to generate stubs can't really be used anymore since it would remove all of these new annotations. We either need to make it smarter and not remove existing annotations, or rely on manual work for now.

@Joao-Dionisio
Copy link
Copy Markdown
Member

so we would need to "finish" everything before we can forbid broad annotations like ˋIncompleteˋ.

That is true, but perhaps we could have an intermediate script that just checks that no new stub errors are introduced. But maybe it's not worth the effort, yes.

On the automated script: yeah, I should do another pass on it someday. Not this weekend, but perhaps the next.

@jonathanberthias
Copy link
Copy Markdown
Contributor Author

@Joao-Dionisio I think this is ready to merge in the current state, I prefer to tackle other topics in other PRs to avoid having too many merge conflicts. Let me know if you see anything wrong!

@Joao-Dionisio
Copy link
Copy Markdown
Member

@jonathanberthias alright, thank you very much! This week might be a bit busy, but I'll try to take a look over the weekend.

@Joao-Dionisio
Copy link
Copy Markdown
Member

Hey @jonathanberthias ! My claude flagged the following:

Found 5 issues:

  1. getLhs is annotated -> None but returns the constraint's left-hand side value (a float). Should be -> float.

def getLeaves(self) -> list[Node]: ...
def getLhs(self, cons: Constraint) -> None: ...
def getLinearConsIndicator(self, cons: Constraint) -> Constraint | None: ...

  1. addConsLocal takes cons: Constraint and returns -> None, but since Change addConsLocal(), addConsNode() to accept ExprCons #1151 the implementation declares ExprCons cons and ends with return pycons (a Constraint). Should be cons: ExprCons and -> Constraint.
    (ExprCons and Constraint are unrelated classes, so passing an expression like x + y <= 5 would be wrongly flagged.)

def addConsLocal(
self,
cons: Constraint,
validnode: Node | None = None,
name: str = "",
initial: bool = True,
separate: bool = True,
enforce: bool = True,
check: bool = True,
propagate: bool = True,
local: bool = True,
dynamic: bool = False,
removable: bool = True,
stickingatnode: bool = True,
) -> None: ...

  1. Same issue in addConsNode: cons: Constraint should be ExprCons, and -> None should be -> Constraint.

def addConsNode(
self,
node: Node,
cons: Constraint,
validnode: Node | None = None,
name: str = "",
initial: bool = True,
separate: bool = True,
enforce: bool = True,
check: bool = True,
propagate: bool = True,
local: bool = True,
dynamic: bool = False,
removable: bool = True,
stickingatnode: bool = True,
) -> None: ...

  1. setBoolParam types value: float, but it sets a boolean parameter (docstring says bool, and SCIPsetBoolParam takes a bool). Should be value: bool — siblings setIntParam/setRealParam are correctly typed.

) -> None: ...
def setBoolParam(self, name: str, value: float) -> None: ...
def setCharParam(self, name: str, value: str) -> None: ...

  1. calcNodeselPriority is annotated -> int, but SCIPcalcNodeselPriority returns SCIP_Real. Should be -> float. Relatedly, createChild's nodeselprio (L967) is typed int but SCIPcreateChild takes SCIP_Real
    nodeselprio, so it should be float too.

def calcChildEstimate(self, variable: Variable, targetvalue: float) -> float: ...
def calcNodeselPriority(
self, variable: Variable, branchdir: Incomplete, targetvalue: float
) -> int: ...

@jonathanberthias
Copy link
Copy Markdown
Contributor Author

Thanks a lot for the review @Joao-Dionisio! I've fixed the findings, indeed I didn't integrate recent changes to the repo correctly, my bad 😅

@Joao-Dionisio
Copy link
Copy Markdown
Member

Okay, I think this should be the last of them:

Over-permissive parameters (None would crash at runtime):

  1. addVarobj: float | None, but only lb/ub are None-handled; obj=None would be passed straight to SCIPcreateVarBasic. Should be obj: float.

def addVar(
self,
name: str = "",
vtype: str = "C",
lb: float | None = 0.0,
ub: float | None = None,
obj: float | None = 0.0,
pricedVar: bool = False,
pricedVarScore: float = 1.0,
deletable: bool = False,
) -> Variable: ...

  1. addMatrixVarobj and pricedVarScore are typed ... | None, but the implementation is Union[int, float, np.ndarray] with no None guard (np.full(..., dtype=float) crashes on None). Drop the
    | None.

def addMatrixVar(
self,
shape: int | tuple[int, ...],
name: str | np.ndarray = "",
vtype: str | np.ndarray = "C",
lb: float | np.ndarray | None = 0.0,
ub: float | np.ndarray | None = None,
obj: float | np.ndarray | None = 0.0,
pricedVar: bool | np.ndarray = False,
pricedVarScore: float | np.ndarray | None = 1.0,
) -> MatrixVariable: ...

  1. getRowParallelismorthofunc: Literal["d", "e", 100, 101], but the value is passed to a C char via integer conversion, so a string raises TypeError; only the ASCII codes work. Should be
    Literal[100, 101] = 101.

def getRowParallelism(
self, row1: Row, row2: Row, orthofunc: Literal["d", "e", 100, 101] = 101
) -> float: ...

Return type missing | None (the implementation has a return None / NULL path):

  1. getConsVals and getConsVars return None when the constraint can't be expressed as a single linear constraint (if not success: return None). Should be list[float] | None and list[Variable] | None.

def getConsVals(self, constraint: Constraint) -> list[float]: ...
def getConsVars(self, constraint: Constraint) -> list[Variable]: ...

  1. getCurrentNodeNode.create(SCIPgetCurrentNode(...)) returns None when not in the solving stage (sibling getters like getBestChild are already Node | None). Should be Node | None.

def getCurrentNode(self) -> Node: ...

Parameter missing | None:

  1. getSolVal (both overloads) — sol: Solution, but the body is (sol or Solution.create(...))[expr] and the docstring says "If None, the current LP/pseudo solution is used" (the sibling getSolObjVal
    is already Solution | None). Should be sol: Solution | None.

@overload
def getSolVal(self, sol: Solution, expr: Union[Expr, GenExpr]) -> float: ...
@overload
def getSolVal(self, sol: Solution, expr: MatrixExpr) -> np.ndarray: ...

Wrong numeric return type (SCIP_Longint maps to Python int, not float):

  1. getCapacityKnapsack returns SCIP_Longint. Should be -> int.

def getCapacityKnapsack(self, cons: Constraint) -> float: ...

  1. getWeightsKnapsack values come from SCIP_Longint*. Should be -> dict[str, int].

def getWeightsKnapsack(self, cons: Constraint) -> dict[str, float]: ...

  1. getParam / getParams — the unions omit int, but SCIP_PARAMTYPE_INT/LONGINT params return Python int. Add int to both.

def getParam(self, name: str) -> bool | float | str: ...
def getParams(self) -> dict[str, bool | float | str]: ...

Return class too narrow:

  1. addMatrixCons is -> MatrixConstraint, but the isinstance(cons, ExprCons) fast-path returns self.addCons(...), a plain Constraint (docstring: "Constraint or MatrixConstraint"). Should be
    MatrixConstraint | Constraint.

def addMatrixCons(
self,
cons: Incomplete,
name: str | np.ndarray = "",
initial: bool | np.ndarray = True,
separate: bool | np.ndarray = True,
enforce: bool | np.ndarray = True,
check: bool | np.ndarray = True,
propagate: bool | np.ndarray = True,
local: bool | np.ndarray = False,
modifiable: bool | np.ndarray = False,
dynamic: bool | np.ndarray = False,
removable: bool | np.ndarray = False,
stickingatnode: bool | np.ndarray = False,
) -> MatrixConstraint: ...

  1. addMatrixConsIndicator — same pattern; the ExprCons path returns self.addConsIndicator(...)Constraint. Should be MatrixConstraint | Constraint.

def addMatrixConsIndicator(
self,
cons: Incomplete,
binvar: Variable | MatrixVariable | None = None,
activeone: bool | np.ndarray = True,
name: str | np.ndarray = "",
initial: bool | np.ndarray = True,
separate: bool | np.ndarray = True,
enforce: bool | np.ndarray = True,
check: bool | np.ndarray = True,
propagate: bool | np.ndarray = True,
local: bool | np.ndarray = False,
dynamic: bool | np.ndarray = False,
removable: bool | np.ndarray = False,
stickingatnode: bool | np.ndarray = False,
) -> MatrixConstraint: ...

Spurious | None in return:

  1. solveBendersSubproblem is -> tuple[bool, float | None], but objective is a cdef SCIP_Real always populated by the C call — never None. Should be tuple[bool, float].

def solveBendersSubproblem(
self,
probnumber: int,
solvecip: bool,
benders: Benders | None = None,
solution: Solution | None = None,
) -> tuple[bool, float | None]: ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants