New model with agent_step! and model_step! #889
Conversation
Codecov Report
@@ Coverage Diff @@
## main #889 +/- ##
==========================================
- Coverage 92.41% 92.25% -0.16%
==========================================
Files 32 32
Lines 2321 2325 +4
==========================================
Hits 2145 2145
- Misses 176 180 +4
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I will review this after we finalize the discussion in #891 |
|
Please do not change any examples in this PR. All examples should work if our deprecations are correctly put in place. |
|
I think we can actually change all the examples in this PR for the reason that we already know that all the deprecated code works since all tests still pass, while I have not applied the new way to any examples/tests. I will do commits that do no touch any souce code so that this is preserved (to see only those it is enough to look inside a commit). |
|
please first run the documentation locally and see that all examples build. before committing any changes to examples. |
|
In fact, not only that. I argue that this PR in genertal should not alter any examples. The examples should be altered in a new PR after this one is merged in. |
|
We need to be extra careful with critical changes such as this one. |
|
ok then let's do it this way |
|
this is the list of the affected functions signatures by this change: ABM since the list is very big, controlling the deprecation warnings is a bit hard, so I'm thinking to only warn the users when they use one of ABM, StandardABM or UnremovableABM and in step!, and use a warning which actually says to them that all of these function don't need the passing of the stepping function, maybe in a general way as: "All of the functions definition which expected at least an agent_step! or a model_step! are deprecated, passing these arguments is not necessary anymore. Pass these functions as keywords arguments (agent_step! = your_agent_step!, model_step! = your_model_step!) inside your model definition (ABM(...), StandardABM(...), UnremovableABM(...))." |
|
Actually I think I can manage to make it work, but anyway I think that saying in each deprecation warning that the change should be applied everywhere the stepping functions are passed seems a good idea to me |
|
If we want to be extra-sure we should do this verification:
|
|
Is this good to review? I'll take over the Tutorial.md file once this is ready. |
|
still not good, but soon it should be |
|
The tests work even in the older version so I think this is ready for review. Running some examples they also work even if I did not update them, so it seems good |
@Tortar I don't know what the hell happened here but this file didn't match the one here: https://github.com/JuliaDynamics/Agents.jl/blob/main/src/simulations/step.jl#L45-L58
|
Okay, I am done here. You should also review the new tutorial and give feedback. I had to update schelling as well, because the schelling example is linked with the tutorial. |
|
Removing the |
Co-authored-by: Tortar <[email protected]>
Sory about that... Could you please fix the tests? |
|
Tests are okay now, I looked at the tutorial and it seems fine to me, can we finally merge it? :-) |
|
We aren't tagging v6 imediatelly after this goes in anyways, so there is no gain in rushing anything. I first need to do a draft for #760 to check if the new model interface makes sense. |
|
The notice in the changelog of this absolutely massive change is too small. I'll update the changelog and then merge. |
Fixes #883.
Still need to be applied to all the library examples and tests and we need to deprecate the visualization functions which require to pass the stepping functions from the outside and create new ones. But this works.
Before finishing it, I'd like to ask @Datseris if this is the right fix.
After this PR we can release 6.0!