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

Improve Schedulers docs #976

Merged
merged 20 commits into from
Feb 18, 2024
Merged

Improve Schedulers docs #976

merged 20 commits into from
Feb 18, 2024

Conversation

Datseris
Copy link
Member

@Datseris Datseris commented Feb 17, 2024

  • All docstrings have been made "correct": they refer to ordering, and not scheduling, and they refer to the IDs, not the agents
  • Deprecated using schedule with the non-default scheduler, as all schedulers are anyways functions of the model, there is no need to pipe them through schedule.
  • Highlighted another usage for the schedulers
  • Remove wording "once per step" from schedulers: this is clear from the StandardABM docstring. Furthermore, how many times scheduling happens in custom scheduling is unknown.

@Tortar
Copy link
Member

Tortar commented Feb 17, 2024

I don't think that removing schedule(model[, scheduler]) is a good idea, how do we manage to filter already removed agents from a custom scheduler? This was the motivation for this feature

@Datseris
Copy link
Member Author

ooooooh shit yeah I have completely forgotten about this....

I will revert everything related to removing schedule.

@Datseris
Copy link
Member Author

@Tortar the way we did it before was suboptimal. Now we check inside step! if an agent exists. it is more efficient than wrapping again the scheduled ids in another Iterators.filter. And now, schedule(model, scheduler) is indeed deprecated.

@Datseris
Copy link
Member Author

Alright so I introduced a new function hasid to make this process simpler.

@Datseris
Copy link
Member Author

tests passs locally

src/core/model_standard.jl Outdated Show resolved Hide resolved
src/submodules/schedulers.jl Outdated Show resolved Hide resolved
src/simulations/step.jl Outdated Show resolved Hide resolved
@Tortar
Copy link
Member

Tortar commented Feb 18, 2024

Seems good anyway to me apart from those details!

@Datseris Datseris merged commit e275f11 into main Feb 18, 2024
4 of 5 checks passed
@Datseris Datseris deleted the schedule branch February 18, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants