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

Add continuous maze example. #216

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mohsen-ghaffari1992
Copy link
Collaborator

@wasowski
Please consider reviewing this example and let me know whether it is what we want.

val TimeHorizon: Int = 2000

def isFinal (s: MazeState): Boolean =
s == MazeState (MAZE_LENGTH, MAZE_WIDTH) || s == MazeState (MAZE_LENGTH, MAZE_WIDTH - 1)
Copy link
Member

@wasowski wasowski Oct 9, 2023

Choose a reason for hiding this comment

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

Should isFinal be defined on MazeState or on MazeObservableState?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to the agent class, it should be maze state.

Copy link
Member

Choose a reason for hiding this comment

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

Then, I think the code is incorrect either here or there. If isFinal is called on observable state, then the set of the final states should be an entire square, not just two points. If isFinal is called by symsim on observable state, then they should require the observable state type, not the system state type? Either side is confused. It does not make sense to check an observable state condition on a continuous state ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then, I think the code is incorrect either here or there. If isFinal is called on observable state, then the set of the final states should be an entire square, not just two points. If isFinal is called by symsim on observable state, then they should require the observable state type, not the system state type? Either side is confused. It does not make sense to check an observable state condition on a continuous state ...

Maybe I wasn't clear, I meant it is defined on MazeState. If it was defined on MazeObservableState, you were right.

case MazeState (_, _) => -1.0


def distort (a: MazeAction): Randomized[MazeAction] = a match
Copy link
Member

Choose a reason for hiding this comment

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

In the continuous, version of the problem it would be more natural to change the distance moved, not the direction. But anyhow, how are you going to treat randomization in the symbolic executor?

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 have an idea that we do not need to do anything about randomization. Let's talk about it in detail in the meeting. Just as a short answer, when there is randomization, it means that instead of a1, the behavior of a2 must be considered. Since we are analyzing the program for all actions, then we should not be worried about randomized actions.

Copy link
Member

Choose a reason for hiding this comment

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

But here a good randomization would not select a different action, but would result in adding random noise (for instance Gaussian) to the distance travelled.

@@ -0,0 +1,19 @@
package symsim
package examples.concrete.continuousmaze
Copy link
Member

Choose a reason for hiding this comment

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

I think the package name should be continuousMaze not continuousmaze.

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 tried to keep it similar to the other packages. e.g. simplemaze, simplebandit, ...
Should I rename it?

@wasowski
Copy link
Member

wasowski commented Oct 9, 2023

I have left my comments, but I am confused why tests for cartpole are failing. Have we added cartpole to main? (I might have simply forgotten...)

@mohsen-ghaffari1992
Copy link
Collaborator Author

@wasowski
Thanks for comments!
I edited the code based on your comments and push the new version.
Regarding to cart pole, yes, we have added it in the main.

@wasowski
Copy link
Member

BTW. Andreas tells me that this example is basically the same thing that is known as the random walk example.

@mohsen-ghaffari1992
Copy link
Collaborator Author

BTW. Andreas tells me that this example is basically the same thing that is known as the random walk example.

Is it?
Random walk is referred to simple maze type of problems according to my knowledge. I googled it a little bit and I still think my understanding is correct.

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