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 incomplete state update issue in the step function of STORM environemnt #60

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

hy-kiera
Copy link
Contributor

The _step() was returning a state where some fields were not updated, leading to inaccuracies in the calculation of done. Specifically, while agent_freezes, agent_positions, and other fields were updated, inner_t remained unchanged in the returned state.

To fix this, I've replaced state_nxt that all fields are updated, ensuring accurate done calculations.

Please review, and feel free to provide any additional feedback or suggestions for further refinement.

@hy-kiera
Copy link
Contributor Author

Hi. @amacrutherford @Aidandos
Could you please review my PR? Thanks.

@amacrutherford
Copy link
Collaborator

Hey! Apologies for the delay on this, @Aidandos had a paper deadline but should get to it over the next few days 😄

@Aidandos
Copy link
Contributor

Hi @hy-kiera . Apologies for the delayed response and thanks for pointing out this issue.
Your fix does fix the inner_t counting, but not the outer_t counting. I edited your PR to fix both.

The counting was broken both in storm_2p.py and storm_env.py.

I added some print statements to the tutorials, showing that inner_t and outer_t are incrementing correctly. It also prints the done logic.
To run the tutorials, simply run
python3 jaxmarl/tutorials/storm_2p_introduction.py
or
python3 jaxmarl/tutorials/storm_introduction.py

@Aidandos Aidandos merged commit cc9f12b into FLAIROx:main Mar 13, 2024
3 checks passed
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