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

Improve unified planning support #309

Merged
merged 21 commits into from
Jan 26, 2024
Merged

Conversation

fteicht
Copy link
Collaborator

@fteicht fteicht commented Jan 24, 2024

This pull request improves the support of unified planning in scikit-decide, notably enabling to solve UP domains with the RLlib solver. It relies on:

  • the recent handling of state-based applicable actions, which are produced by unified planning domains, in the RLlib solver ;
  • the automatic translation to/from unified planning states and actions from/to numpy arrays that can be handled by RLlib.

@fteicht fteicht added the enhancement New feature or request label Jan 24, 2024
@fteicht fteicht requested a review from g-poveda January 24, 2024 21:16
@fteicht fteicht self-assigned this Jan 24, 2024
Copy link
Collaborator

@g-poveda g-poveda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand those commit, not related to up isn't it ?
is it some results of pre-commit or refactor you made ?

@@ -9,15 +9,14 @@


@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher")
def test_up_bridge_domain():
def test_up_bridge_domain_random():
noexcept = True

try:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be the opportunity to remove the try except here ?

@@ -420,7 +420,7 @@ def get_elements(self) -> Iterable[T]:
return self._elements

def sample(self) -> T:
return self._elements[super().sample()]
return self._to_elements[super().sample()]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it a bug you found during this PR implementation ?

Copy link
Collaborator Author

@fteicht fteicht Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I found this bug when calling the sample method of SetSpace in the UP bridge unit tests.

@fteicht
Copy link
Collaborator Author

fteicht commented Jan 26, 2024

I don't understand those commit, not related to up isn't it ? is it some results of pre-commit or refactor you made ?

There are 2 kinds of commits here:

  • Python imports reordering in order to comply with linting rules for Python
  • Adding missing solvers and domains to pyproject.toml which I spotted when adding the UP bridge ones

Copy link
Collaborator

@g-poveda g-poveda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment :)

@fteicht fteicht merged commit 3e5bacb into airbus:master Jan 26, 2024
42 checks passed
@fteicht fteicht deleted the improve-up-domain branch January 26, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants