API: use `__new__` in `.create` to remove `__init__` totally

[Zeroto521]

Is your feature request related to a problem? Please describe.

__init__ method is not used in PySCIPOpt due to the SCIP pointer.
So create comes out to instead of __init__.
And it could still see __init__ in .create.

The chain to generate an instance via __init__.

1. Run __new__ and return a empty instance
2. Run __init__ and return this instance.

Describe the solution you'd like

use __new__ to generate a instance. It could avoid running any code in __init__.
And a little speed up via skipping the checking.

cdef class Solution:

    def __init__(self):
        raise ValueError

    @staticmethod
    cdef create(SCIP* scip, SCIP_SOL* sol):
        if scip == NULL:
            raise Warning("cannot create Solution with SCIP* == NULL")

        cdef Solution res = Solution.__new__(Solution)
        res.sol = sol
        res.scip = scip
        return res

use __init__ to generate a instance

cdef class Solution:

    def __init__(self, raise_error = False):
        if not raise_error:
            raise ValueError

    @staticmethod
    cdef create(SCIP* scip, SCIP_SOL* scip_sol):
        if scip == NULL:
            raise Warning("cannot create Solution with SCIP* == NULL")
        sol = Solution(True)
        sol.sol = scip_sol
        sol.scip = scip
        return sol

[Joao-Dionisio]

Hey @Zeroto521 , can you please explain the benefit of removing init a bit more? is it because it's not really doing anything? Thank you for all your work in the repo, by the way!

[Zeroto521]

__new__ could directly create an instance without type checking, as follows.
And it has a little performance gain.

from __future__ import annotations

from time import time

Variable = int


class Term:
    def __init__(self, *vars: Variable):
        for var in vars:
            if type(var) is not Variable:
                raise TypeError(f"Expected Variable, got {type(var)}")
        self.vars = vars
        self._hash = hash(self.vars)

    def create(*vars: Variable) -> Term:
        res = Term.__new__(Term)
        res.vars = vars
        res._hash = hash(res.vars)
        return res

    def __repr__(self) -> str:
        return f"Term{self.vars}"


if __name__ == "__main__":
    t1 = Term(1, 2, 3)
    print(f"{t1=}")
    t2 = Term.create(1, 2, 3)
    print(f"{t2=}")

    start = time()
    for _ in range(10_000):
        Term(1, 2, 3, 4, 5)
    end = time()
    print(f"Time taken for __init__: {end - start:.4f} seconds")

    start = time()
    for _ in range(10_000):
        Term.create(1, 2, 3, 4, 5)
    end = time()
    print(f"Time taken for create  : {end - start:.4f} seconds")

    # t1=Term(1, 2, 3)
    # t2=Term(1, 2, 3)
    # Time taken for __init__: 0.0060 seconds
    # Time taken for create  : 0.0040 seconds

We explore the Solution class or others for the user. But we don't want them to create an instance via Solution() (__init__).
All class instances should be created by the Model class, not __init__.
We could use __new__ feature to create an instance and hide the __init__ entry.

cdef class Solution:

    def __init__(self):
        raise ValueError

    @staticmethod
    cdef create(SCIP* scip, SCIP_SOL* sol):
        ...

[Joao-Dionisio]

I think the performance gain is too marginal to justify the risk in changing something so fundamental. For the user, there would be no benefit whatsoever, and for the developer, it's not something we're really concerned with. I'd then vote in favor of closing this issue.

[Zeroto521]

Performance gain is main reason for Term. These classes init method have a lot of logic.
Safety is main reason for Expr, GenExpr, Solution, Model. These classes are explored and this could avoid calling them via init