Fixed move_agent_to_one_of to handle empty positions correctly

[aarav-shukla07]

Summary

This PR improves the move_agent_to_one_of method in space.py by ensuring that agents do not move when no valid positions are available. Previously, if an empty list was provided, the agent would still move unpredictably, leading to incorrect behavior.

Bug / Issue

Fixes part of issue #1903.

Issue Details:

• The function previously moved agents even when no valid positions were available.
• Tests such as test_move_agent_empty_list and test_move_agent_no_eligible_cells failed due to incorrect movement.

Expected Behavior:

• If no valid positions exist, the agent should stay in place instead of moving randomly.
• The function should handle empty lists gracefully without causing unintended movement.

Implementation

Added an early return condition to move_agent_to_one_of when pos is empty.
If handle_empty is "warning", it logs a warning but does not move the agent.
If handle_empty is "error", it raises an exception instead of moving the agent randomly.
Ensured that the agent only moves when a valid target position exists.

Testing

Re-ran full test suite:

pytest tests/test_space.py

Additional Notes

• This implementation follows best practices by keeping the logic simple and efficient.
• The solution aligns with the reviewer’s request for an elegant way to place agents on empty cells.
• Further refinements (if any) can be discussed with maintainers for future improvements.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔵 +2.8% [+2.0%, +3.7%]	🔵 +0.8% [+0.7%, +1.0%]
BoltzmannWealth	large	🔵 +17.9% [-2.2%, +54.1%]	🔵 +3.4% [+0.8%, +5.5%]
Schelling	small	🔵 +0.0% [-0.2%, +0.3%]	🔴 +3.3% [+3.2%, +3.5%]
Schelling	large	🔵 -2.1% [-7.5%, +0.9%]	🔴 +7.4% [+6.2%, +8.7%]
WolfSheep	small	🔵 +1.4% [+1.0%, +1.7%]	🔵 +0.6% [+0.4%, +0.7%]
WolfSheep	large	🔵 +0.9% [+0.4%, +1.3%]	🔵 +2.7% [+1.3%, +4.1%]
BoidFlockers	small	🔵 +0.6% [-0.2%, +1.3%]	🔵 -0.4% [-0.6%, -0.2%]
BoidFlockers	large	🔵 -0.0% [-0.8%, +0.6%]	🔵 -0.4% [-0.7%, -0.1%]

[EwoutH]

Thanks for the PR!

You made an improvement to the old space, which is currently maintenance only. So I don’t know if we can accept it, but I will let the other maintainers weigh in.

With older issues it’s always worth checking in if they are still relevant.

The PR looks well structured by the way!

[aarav-shukla07]

Hey @EwoutH

Thanks for your feedback!

I understand that the old space is in maintenance mode. Should I check if a similar improvement is needed in the newer implementation?
Also, let me know if there's anything I can do to make this PR more useful for the current system.

Looking forward to hearing from the other maintainers!

Thanks again!

[EwoutH]

It happens to the best of us :)

Should I check if a similar improvement is needed in the newer implementation?

@quaquel I think this feature is already covered by the new API, or is it not yet?

[quaquel]

I think this feature is already covered by the new API, or is it not yet?

Depends a bit on how you define "this feature". CellCollection has a select_random_cell method. The closest behavior is not readily available but is trivial to do yourself. Just take the absolute difference in coordinates between two cells and sum across coordinates (so using city block distance).

[EwoutH]

Thanks!

@aarav-shukla07 you could study the current discrete space API, and if you come up with an idea to improve/extend it, propose it to us :)

[aarav-shukla07]

Hey @EwoutH @quaquel ,
Thanks for the clarification and insights!

I'll study the current discrete space API in more detail and see if I can come up with any ideas to improve or extend it.

If I find a meaningful improvement, I'll propose it to you for discussion before implementing it.

Thanks again for your guidance! Looking forward to contributing further.

[EwoutH]

Perfect, I’m closing this PR, looking forward to your future work!