space: Let move_agent choose from multiple positions

[EwoutH]

This PR enables the move_agent method to take a list of potential positions and offering two selection strategies: random and closest.

This allows any subset of potential positions to be inputted, like an neighborhood, list of empty cells, etc.

Furthermore, it will help with #1898 when cells are selected based on properties.

New features

1. Flexible Position Handling: The move_agent method can now accept a single position or a list of possible positions for agent movement.

2. Selection Strategies:

   • random: Selects a position randomly from the provided list.
   • closest: Chooses the position closest to the agent's current location. In case of ties, a position is selected randomly from the closest ones.

Usage examples

1. Moving to a Random Position:

   space.move_agent(agent, possible_positions, selection='random')

2. Moving to the Closest Position:

   space.move_agent(agent, possible_positions, selection='closest')

Limitations

• This currently only works for Grids, not for ContiniousSpace.

[EwoutH]

@jackiekazil @tpike3 @Corvince @rht @wang-boyu @quaquel please review

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9495a5a) 77.53% compared to head (5bab49e) 79.62%.
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1920      +/-   ##
==========================================
+ Coverage   77.53%   79.62%   +2.08%     
==========================================
  Files          15       15              
  Lines        1015     1124     +109     
  Branches      221      244      +23     
==========================================
+ Hits          787      895     +108     
- Misses        197      198       +1     
  Partials       31       31              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Corvince]

I definitely like this PR and this is something we should implement.
However the first thing that comes to my mind is that this would be a breaking change and could potentially really break a lot of models, because I don't think it's uncommon to pass the coordinates as a list of x, y instead of a tuple. It's probably also used in some of the examples. Maybe there is a way to circumvent this issue.

[Corvince]

One thing that could be done is to introduce this as a now function that is simply called "move" and at some time deprecate the other one.

[EwoutH]

because I don't think it's uncommon to pass the coordinates as a list of x, y instead of a tuple.

Good point, I also noticed this in testing.

What if we check if it's a sequence of numbers (sequence of int/float)? That way it would catch lists coordinate input, but still allow multiple coordinates (which would be sequences of sequences).

[EwoutH]

Implemented in 0e8e42f. None of the test are now needed to be modified.

We should still steer to inputting tuples, it's also type hinted that way. But that can be another effort.

[EwoutH]

This can be quite slow for long lists of agents. Could we just check the first element?

[Corvince]

That sounds correct. Either the first element is just a number or another iterable.

[EwoutH]

Implemented, all tests pass.

We have to think about how to process empty lists of possible positions. Either:

• Don't allow it (by crashing) (current behavior)
• Don't move the agent

[Corvince]

Good question. My first instinct would say don't move but issue a warning. I guess most of the time something went wrong when trying to move to an empty list of cells, but crashing the program seems a bit harsh.

[EwoutH]

Having thought about it a bit, let’s keep it this way for now. Before this PR, you where required to input a location and your agent would always move. Now, that’s still the case, only multiple is also allowed.

If empty lists become an issue that we can’t solve in usage, we can revisit this.

[EwoutH]

Still thinking about the empty lists case. Currently, if you think you might have no target locations, you have to check manually if this is the case:

possible_positions = some_selection_function()
if possible_positions >= 1:
  move_agent(agent, possible_positions)

Advantages:

• Explicit
• You can do a else if no location if found
• Maybe easier to debug

Disatvantages:

• Added boilerplate code

We could add a allow_no_move boolean to move_agent, which if true allows to input an empty list, which results in the agent not moving (either with or without warning).

However, then we fundamentally change the behavior of move_agent: Instead of always moving, we now maybe move the agent.

Another solution could be just to create a new method for these cases, and leave move_agent as it originally was. Then move_agent requires a single position and you can be sure your agents moves there, while move_agent_to_one_of moves an agent to one of a list. If the list if empty, it doesn't move (and we could add a bool for a warning if that happens).

[quaquel]

Another solution could be just to create a new method for these cases, and leave move_agent as it originally was. Then move_agent requires a single position and you can be sure your agents moves there, while move_agent_to_one_of moves an agent to one of a list. If the list if empty, it doesn't move (and we could add a bool for a warning if that happens).

I like the clarity of move_agent_to_one_off and keeping it separate from move. In this case, you can keep the default behavior as random, like you want. I am unsure about the behavior of the following two cases.

1. Empty list; I am inclined to raise an error in this case. You cannot move, so it is an error, and errors should not be passed over silently. It is up to the user to catch the exception and handle it properly. Clearly, this must be documented well.
2. list with one item; in principle, you can simply handle this identically to a list larger than one.

[EwoutH]

Fully refactored the PR. There is now a new method move_agent_to_one_of (suggestions for other names are welcome), and the move_agent method is unchanged. Docs are updated and tests pass.

Random is the default selection method and by default, empty lists are allowed. You can optionally enable warnings or errors on empty list input.

Curious what everybody thinks!

[jackiekazil]

Is there an example that demonstrates this functionality change beyond the testing?
(I am thinking of how people discover this.)

[tpike3]

LGTM! Thanks @EwoutH

[tpike3]

Is there an example that demonstrates this functionality change beyond the testing? (I am thinking of how people discover this.)

@jackiekazil I can add this to the CE tutorial. @EwoutH if you want to add examples to the How To Guide when you get the chance this will wrap it up I think

[EwoutH]

Thanks for merging! Also nice to see that we can have critical discussions and still keep a certain velocity. It feels like a nice balance and flow in general.

Agreed on the examples. I will try to rebase #1898 soon after Christmas. And then I think we can have an amazing release to start 2024!

Have a great Christmas everyone!