JuliaDynamics / Agents.jl

Agent-based modeling framework in Julia
https://juliadynamics.github.io/Agents.jl/stable/
MIT License
717 stars 115 forks source link

Move `Models` submodule into test folder #939

Closed fbanning closed 7 months ago

fbanning commented 8 months ago

This PR does three things:

  1. It moves the Models submodule out of the main Agents package and into the test folder. This makes it available internally for us for the documentation and the test suites but doesn't expose it to the users anymore. The module is also renamed to AgentsTestModels to avoid any confusions and/or potential naming collisions.
  2. It extends the predefined models that are available via the Models submodule. The rabbit_fox_hawk model (from our examples section) has been added to the set of available predefined models and can be accessed via the AgentsTestModels.rabbit_fox_hawk() function.
  3. It makes sure that our model initialisation functions only ever return model and not agent_step! and model_step! as well because they are included in the ABM struct now.

Important note: Adding the rabbit_fox_hawk model in its current form requires us to add FileIO package as a dependency. Is that acceptable to you, @Datseris?

Tortar commented 8 months ago

A better solution in my opinion is to move the entire models folder in the test folder (or anyway outside of src), and add fileIO as a dep of the tests only, in the end we use these models only for testing if I'm not mistaken

Datseris commented 8 months ago

That is correct, we no longer mention Models in the documentation because providing pre-defined models is deprecated. We use them in the tests but what @Tortar says is the way to go I think.

fbanning commented 7 months ago

I've changed the direction of this PR now to reflect your suggestions to fully remove the Models submodule. It is now called AgentsTestModels and is located inside the test folder from now on. It is thus not exposed to or accessible by the users anymore.

in the end we use these models only for testing if I'm not mistaken

We also reference it in the example section of our documentation but those references can easily be replaced or removed which I've done now.

codecov-commenter commented 7 months ago

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (668f1c0) 80.77% compared to head (ffb9164) 85.72%. Report is 8 commits behind head on main.

Files Patch % Lines
src/simulations/collect.jl 73.33% 12 Missing :warning:
src/core/space_interaction_API.jl 50.00% 2 Missing :warning:
src/submodules/schedulers.jl 50.00% 2 Missing :warning:
src/spaces/discrete.jl 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #939 +/- ## ========================================== + Coverage 80.77% 85.72% +4.94% ========================================== Files 45 36 -9 Lines 2955 2543 -412 ========================================== - Hits 2387 2180 -207 + Misses 568 363 -205 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Tortar commented 7 months ago

yes, in my opinion it is the cleanest solution to have them in the AgentsExamplesZoo :-)

Il Mar 19 Dic 2023, 09:25 Frederik Banning @.***> ha scritto:

@.**** commented on this pull request.

In examples/agents_visualizations.jl https://github.com/JuliaDynamics/Agents.jl/pull/939#discussion_r1431068495 :

-sir_model = Models.sir() +using .AgentsTestModels: sir #hide +sir_model = sir()

does it mean that the user won't be able to copy-paste the code in the example?

Not with a simple click but if they just go the the link mentioned in the docs themselves, they can copy the model quite easily.

maybe we can call it this way Agents.include_test_models()

Creating such a helper function is just a more complicated way to have the Models submodule in the package itself. I'm against such a half-baked solution. Either we don't expose those predefined models to the users at all or we put all of this in AgentsExampleZoo. I've actually thought about the latter quite a bit while doing this PR and I would be in favour of that. Give me your opinion and we'll decide what to do. :)

— Reply to this email directly, view it on GitHub https://github.com/JuliaDynamics/Agents.jl/pull/939#discussion_r1431068495, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQH6VX5Z3GYVDXEBVNPH3MLYKFFP7AVCNFSM6AAAAAA7ZLEY4GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOBYGMZTINBRHA . You are receiving this because you were mentioned.Message ID: @.***>

Tortar commented 7 months ago

I updated this pr given that now models are present in the AgentsExampleZoo :-)

to me it seems good, so I merge it

Datseris commented 6 months ago

thanks for this guys!