-
Notifications
You must be signed in to change notification settings - Fork 0
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
Cumulative wells implementation #1
Comments
Hi Olly, I believe you're right! My version will need a reimplementation to account for the cumulative depth summation. I'll look into a reimplementation of this once the dissertation marking has passed. |
Excellent! As a result, I expect your agent's mean performance to increase. |
Hi Ben, Apologies if this comes across as critical. I was looking at your implementation for inspiration and realised that it's incorrectly treating some of the squares as part of a well when they should not be — "A square that is part of a well is an empty square that can be reached from above with full squares on the sides." (Link). I wonder if a solution could be obtained by excluding the squares that are below the highest full squares in their respective columns and using numpy.cumsum to account for the cumulative depth summation. |
I have written a solution and the tests in my test suite still pass. Feel free to utilise/critique it. Apologies that I haven't created a PR — I wasn't able to set up a copy of your codebase for testing. |
Hi Ben,
Your implementation of cumulative wells sums the depths of all wells.
gym-mdptetris/gym_mdptetris/envs/feature_functions.py
Lines 99 to 145 in 1a47edc
However, I reckon that you should sum i from i=1 to d(w) for all wells w, where d(w) is the depth of w, then add the sums. You can find a more precise definition here.
I believe that my implementation is correct, although there is probably a faster way using NumPy, which you could help me out with here?
Olly :)
The text was updated successfully, but these errors were encountered: