industrial-optimization-group / DESDEO

An open source framework for interactive multiobjective optimization methods
https://desdeo.it.jyu.fi
28 stars 22 forks source link

Solvers are now classes, but many things are expecting CreateSolverType functions #118

Closed juropo closed 1 week ago

juropo commented 2 months ago

What is the current behavior?

Describe the solution you'd like

What is the motivation/use case for changing the behavior?

Describe alternatives you've considered

Additional context

gialmisi commented 1 month ago

Indeed, this should be fixed. As far as I know, this is mostly an issue with the types, which should be definitely checked. Can you elaborate what your mean by

Right now, the code does not work, so something needs to be changed.

I think the changes you suggested

Remove the CreateSolverType and add SolverType (or something) that tells what other code can expect from the solvers.

Fix all the errors that result from removal of CreateSolverType by updating the code to use the solver classes.

should be enough.

For instance, in desdeo/tools/generics.py, CreateSolverType = Callable[[Problem, SolverOptions], Callable[[str], SolverResults]] can be replaced with a generic class, e.g., SolverBase, which could look like

from abc import ABC, abstractmethod

class BaseSolver(ABC):

    @abstractmethod
    def solve(self, problem: str) -> SolverResults:
        pass

I would keep this as simple as possible for now. If we need more guarantees from a solver, other than the solve method, then we can update this. This base class should be compatible with all current solvers.

@Matskuu I am assigning this to you. If you have questions, feel free to ask for more details.

juropo commented 1 month ago

Indeed, this should be fixed. As far as I know, this is mostly an issue with the types, which should be definitely checked. Can you elaborate what your mean by

Right now, the code does not work, so something needs to be changed.

I think I worded that poorly. I'm not aware of any errors that arise because of this. I just meant that the code does not work as it claims to. I'll update the issue to reflect this better.

juropo commented 1 week ago

I believe this has been resolved