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

fixing bugs. case 5 runs. hits infinite loop #19

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

connoralittle
Copy link
Collaborator

Updates on fixing Pr2

src/deadend.cc Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all_fire_context still needs to be defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

allocate reintroduced

Copy link
Contributor

Choose a reason for hiding this comment

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

Was it removed by me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was. It used to be allocating an array but the data structure is a vector now. I assume that is partly why

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Task changed to root task. Comparison between nodes asserts the tasks must be the same. Unsure which change would need to be made

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sitting on it, removing the assert seems to be the right choice

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to where the assert occurs? This in the FD core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

task_proxy.h line 174. The assert ensures factproxies only compare when they have the same task. The taskproxy produces operators with facts from one task and the lazy search algorithm produces facts with another, making them incomperable.

src/simulator.cc Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Operators were being overwritten. Temporary fix is to assign them to a new pointer.

void _allocate(int size) {
vars.reserve(size);
}
void _deallocate() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the memory be released?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I may be wrong, but I believe the C++ vector should automatically deallocate everything when it goes out of scope. Looking it up reserve doesn't seem to require a free method

// PRP.general.conditional_mask[nondet_index]->push_back(cond.var);
// }
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it worth keeping this unworking code around for when we re-create the mask?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still have that code in a local repository need be. I removed it to clean up the code. Conditional mask is put in the same places but I don't know if it has been tested yet.

@connoralittle
Copy link
Collaborator Author

connoralittle commented Sep 4, 2024

One change that I found but hasn't been pushed yet:
Engine needs to reset status and solutionfound, private variables. This leaves two options to my eyes.
change the initialization method in fast downward or recreate the engine every search. The former is more changes to fd and the latter is an expensive operation. Testing both they both seem to work in the sense they reach the same bug later on.

…oth as an ordinal and as an index, and by setting it to represent both it seems to fix several bugs down the line. C++ never threw an index out of bounds exception, so it wa hard to deduce
@connoralittle
Copy link
Collaborator Author

A quick update as to the next step. Case 5 is failing on retries as the popped node already satisfies the goal state and so returns an empty plan. Again cpp does not tell me I am getting index out of bounds errors and instead allows access to garbage memory. The goal is to identify what is causing this behaviour.

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.

2 participants