Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AStar solver not implementing _solve_domain as expected #218

Closed
g-poveda opened this issue Oct 20, 2022 · 7 comments
Closed

AStar solver not implementing _solve_domain as expected #218

g-poveda opened this issue Oct 20, 2022 · 7 comments
Assignees
Labels
invalid This doesn't seem right question Further information is requested

Comments

@g-poveda
Copy link
Collaborator

the AStar solver :( skdecide/hub/solver/astar/astar.py ) doesn't seem to solve actually the domain when calling
_solve_domain method. It just initialize internal solver (coded in cpp) but don't run the solve. This seems different from other hub solvers and scikit-decide api in general.
(currently, the actual solving is when calling solve_from(memory))

@g-poveda g-poveda added the invalid This doesn't seem right label Oct 20, 2022
@nhuet
Copy link
Contributor

nhuet commented Oct 9, 2023

@g-poveda Could you develop about

This seems different from other hub solvers

?

Because, it seems to be the same behaviour for other c++ solvers (probably there are all wrong in that by the way):

  • aostar
  • bfws
  • ilaostar
  • iw
  • lrtdp
  • martdp
  • mcts
  • riw

The only c++ solver that seem to implement a real _solve_domain():

  • mahd

@g-poveda
Copy link
Collaborator Author

g-poveda commented Oct 9, 2023

@nhuet The pure python solvers in the hub :
like ars, lazy_astar, lrtastar, cgp... all are launching the solving of the domain in the _solve_domain function, whereas the c++ ones not. I guess/think that the correct way is to do the solve in the _solve_domain.

@fteicht
Copy link
Collaborator

fteicht commented Oct 9, 2023

I would challenge that one and call for a discussion.
We could also argue the opposite, i.e. that pure python solvers don't follow the c++-based solvers.
The thing is that, with the python solvers's api, we have to define new domains each time we want to change just the initial state of the domain. Is this really what we want?

But in any case, I agree that we should agree on a common consistent behaviour but first discuss what should be this behaviour.

By the way, the same is valid for the solvers' constructors which have different signatures between pure python and C++-based solvers

@fteicht fteicht added the question Further information is requested label Oct 9, 2023
@g-poveda
Copy link
Collaborator Author

g-poveda commented Oct 9, 2023

Ok agree @fteicht , i think i raised this issue to get more consistency and actually the solve_from() probably has its advantages.

@fteicht
Copy link
Collaborator

fteicht commented Nov 14, 2023

Agree. I propose to change the behaviour of the C++ solvers so that _solve_domain will actually call the solver if the domain provides a default initial state. If not, _solve_domain will continue just initializing the solver, whose search will be launched only when calling _solve_from (which is the current behaviour of all the C++ solvers).

@fteicht
Copy link
Collaborator

fteicht commented Feb 20, 2024

Also, the _solve_from method should have the same signature as the _is_solution_defined_for, i.e. observation: D.T_agent[D.T_observation].

@nhuet
Copy link
Contributor

nhuet commented May 21, 2024

Solved by #320.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants