JuliaDynamics / Agents.jl

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

Recreate add_agent!(agent,...) + new constructor for agents #946

Closed Tortar closed 6 months ago

Tortar commented 7 months ago

Closes #945

codecov-commenter commented 7 months ago

Codecov Report

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

Comparison is base (433faa4) 92.18% compared to head (ec31ee1) 92.15%. Report is 6 commits behind head on main.

:exclamation: Current head ec31ee1 differs from pull request most recent head 53bc394. Consider uploading reports for the commit 53bc394 to get more accurate results

Files Patch % Lines
src/core/agents.jl 80.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #946 +/- ## ========================================== - Coverage 92.18% 92.15% -0.04% ========================================== Files 33 33 Lines 2330 2344 +14 ========================================== + Hits 2148 2160 +12 - Misses 182 184 +2 ```

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

Tortar commented 7 months ago

ok, I realized we only need to reabilitate add_agent_pos!, the other usages are a bit superfluous with the other add_agent! methods in place. I also re-added add_agent_single! because it is a bit more useful as a function.

In general, add_agent_pos! is an unfortunate name nonetheless because it could well be just add_agent!, which would be better for example with a nothing space, and it seems the correct name in any case. Unfortunately we used this version for adding the agent to a random position in the past, and so changing it to instead add it at the own position of the agent is a breaking change. What I'm thinking is that we can send to the direct users of add_agent_pos! a future warning saying that this function will be renamed add_agent! (maybe in 6.2?). Or keep it like this, even if it doesn't seem the right name. What do you think?

I also added the constructors accepting as the first argument the model instance so that it creates agents the way it should. I will document it in the agent macro docstring.

Datseris commented 7 months ago

In general, add_agent_pos! is an unfortunate name nonetheless because it could well be just add_agent!, which would be better for example with a nothing space, and it seems the correct name in any case. Unfortunately we used this version for adding the agent to a random position in the past, and so changing it to instead add it at the own position of the agent is a breaking change. What I'm thinking is that we can send to the direct users of add_agent_pos! a future warning saying that this function will be renamed add_agent! (maybe in 6.2?). Or keep it like this, even if it doesn't seem the right name. What do you think?

Ι agree with you. This is an unfortunate naming. My worry however is that this function is so fundamental, and so integrated into the package and all its usage, that changing would be just too much.

What I think is the best solution is to find a better name alltogether for the function. How about add_agent_own_pos? This signifies that the agent is added to its own position.

I believe that on average it is more common that the user wants agents added to random positions rather than a specified position hence I would argue that the less verbose function add_agent should stay as is and do the random position unless specified otherwise.

What do you think of that?

This means that add_agent_pos is deprecated and throws a warning for the new function name.

Tortar commented 7 months ago

Thought about this a bit, and I agree with you that add_agent_own_pos! is a little bit better so I guess it's okay to have both add_agent!(agent, [, pos], model) and add_agent_own_pos!(agent, model). Will update this PR in this direction.

Datseris commented 6 months ago

@Tortar what is the status here? is this PR ready for review?

Tortar commented 6 months ago

Need still to finish this, I'll do it today

Tortar commented 6 months ago

Should be okay now!

Tortar commented 6 months ago

ok very good :D, same tests failing as in #875, locally everything works as in the other PR so we can merge both, but something is strange on Github and we should try to understand what