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

Issue454 - move computation of negated axioms to search component #220

Merged
merged 40 commits into from
Jul 19, 2024

Conversation

salome-eriksson
Copy link
Contributor

No description provided.

Copy link
Contributor Author

@salome-eriksson salome-eriksson left a comment

Choose a reason for hiding this comment

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

I added some comments on the code where I try to explain why I implemented certain things the way I did.

src/search/CMakeLists.txt Outdated Show resolved Hide resolved
src/search/heuristic.h Outdated Show resolved Hide resolved
src/search/landmarks/landmark_heuristic.cc Outdated Show resolved Hide resolved
src/search/heuristics/cea_heuristic.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@roeger roeger left a comment

Choose a reason for hiding this comment

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

First round of comments. I haven't looked at everything, yet.

src/search/landmarks/landmark_heuristic.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.h Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
@aibasel aibasel deleted a comment from roeger Jul 3, 2024
@aibasel aibasel deleted a comment from roeger Jul 3, 2024
@aibasel aibasel deleted a comment from roeger Jul 3, 2024
@aibasel aibasel deleted a comment from roeger Jul 3, 2024
Copy link
Contributor

@roeger roeger left a comment

Choose a reason for hiding this comment

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

Hi @salome-eriksson,

I added another round of comments.

For the record, we talked about two more general aspects in person:

  • it is a bit confusing that parts of the implementation have a perspective of a fact that can be true or false (e.g. "needed negatively") and other parts use the perspective of a binary finite-domain variable that has a default value and the other value.
  • @speckdavid suggested to add an option to the affected heuristics to use the overapproximation for all derived variables that are needed negatively (weakening the heuristic but saving the time for the transformation).

Best,
Gabi

src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/heuristics/cea_heuristic.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.h Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.h Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/negated_axioms_task.cc Outdated Show resolved Hide resolved
Copy link
Member

@FlorianPommerening FlorianPommerening left a comment

Choose a reason for hiding this comment

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

I did a light review on the auxiliary parts of the code (CMake, Options, etc.) I didn't look at the actual implementation in src/search/tasks and src/translate.

What I didn't quite get from the documentation is what happens if I don't use the option simple_default_value_axioms. Will axioms just behave "correctly" (whatever this means) without the option and once I set this option, I approximate this behavior in a way that can only weaken heuristics?

src/search/CMakeLists.txt Outdated Show resolved Hide resolved
src/search/CMakeLists.txt Show resolved Hide resolved
src/search/axioms.cc Show resolved Hide resolved
src/search/heuristics/cea_heuristic.cc Outdated Show resolved Hide resolved
src/search/landmarks/landmark_heuristic.cc Outdated Show resolved Hide resolved
@salome-eriksson
Copy link
Contributor Author

I did a light review on the auxiliary parts of the code (CMake, Options, etc.) I didn't look at the actual implementation in src/search/tasks and src/translate.

What I didn't quite get from the documentation is what happens if I don't use the option simple_default_value_axioms. Will axioms just behave "correctly" (whatever this means) without the option and once I set this option, I approximate this behavior in a way that can only weaken heuristics?

The negated axioms are always "correct" in the sense that the heuristic will be safe and stay admissible if it should be (for example hmax). But sometimes the conditions under which a derived variable achieves its default value are just (over)approximated, making the heuristic weaker. If the option is set, then all negated axioms are just overapproximated, which is much faster to compute, but can lose a lot of information. If the option is not set, then the overapproximation is only done for some axioms (those whose derived variables have cyclic dependencies, see issue453 for details).

Copy link
Contributor

@roeger roeger left a comment

Choose a reason for hiding this comment

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

Hi @salome-eriksson,

I had another look at the changes and only left a few comments on cosmetics. From my side, the code in principle is ready to merge.

Best,
Gabi

src/search/tasks/default_value_axioms_task.cc Show resolved Hide resolved
src/search/tasks/default_value_axioms_task.cc Outdated Show resolved Hide resolved
src/search/tasks/default_value_axioms_task.cc Outdated Show resolved Hide resolved
@salome-eriksson salome-eriksson merged commit f75018f into aibasel:main Jul 19, 2024
11 of 12 checks passed
@salome-eriksson salome-eriksson deleted the issue454 branch July 19, 2024 11:30
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.

3 participants