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

Fix time management in robots and marbles notebook 4 #33

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

Conversation

AntoineRondelet
Copy link

@AntoineRondelet AntoineRondelet commented Feb 17, 2021

The notebook 4 starts by the following:

In this notebook, we'll cover cadCAD's support for modelling non-deterministic systems and Monte Carlo simulations. But first let's copy the base configuration with which we ended Part 3. Here's the description of that system:
The robot and the marbles
Picture a box (box_A) with ten marbles in it; an empty box (box_B) next to the first one; and two robot arms capable of taking a marble from any one of the boxes and dropping it into the other one.
The robots are programmed to take one marble at a time from the box containing the largest number of marbles and drop it in the other box. They repeat that process until the boxes contain an equal number of marbles.
The robots act asynchronously; robot 1 acts once every two timesteps, and robot 2 acts once every three timesteps

This suggests (to me at least :) ) that the first part of this notebook will follow the last part of the notebook 3.
Nevertheless, the results of the simulation look different.

In fact, after a look at the code across these 2 sections (last section of notebook 3 and first section of notebook 4) it seems that time is managed slightly differently (justifying the different simulation results).

I'm not entirely sure how the backend of cadCAD operates, but I suspect that steps are shifted by one 1 w.r.t to s['timestep'] (i.e. the first step of the simulation is done at time 0). In fact, AFAICT, in notebook 3, the robots do not start operating at the system's starting time (the def get_current_timestep(cur_substep, s) function seems to avoid that and re-align steps and timesteps), while this is not the case anymore in the first section of the notebook 4, where def get_current_timestep(cur_substep, s) is not used anymore in the robot_arm_1 and robot_arm_2 policies and where s['timestep'] is used directly instead. This means that in the notebook 4, both policies will return robot_arm(params, step, sH, s) if s['timestep'] = 0 because 0%robots_periods[_robotId - 1] will trivially return 0 in both cases. Hence, both robots will move marbles at the same time for timestep t = 0 (which again, seems to be enforced not to happen like that in the last section of the notebook 3).

I am not sure if this slight change in the model is intentional or not, but as far as I understand the first introductory part of the notebook 4 (quoted above) this does not appear to be desired.

This PR fixes this and re-introduces the get_current_timestep function in the first section of the notebook 4.
Another way to fix this, would be to rephrase the model specification.

Please let me know if I missed something,
Thanks!

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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.

1 participant