Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored docs for Introductory Tutorial #2511

Merged
merged 6 commits into from
Nov 25, 2024
Merged

Conversation

Spartan-71
Copy link
Contributor

Summary

Fixed minor logical error in give_money function of MoneyAgent class.
Fixed code inconsistency.

Bug / Issue

Earlier MoneyAgent was able to transfer the money to itself.

Implementation

Now MoneyAgent self has been removed from the cellmates list and the condition to transfer the money is modified to if len(cellmates) > 0

Testing

Additional Notes

@EwoutH
Copy link
Member

EwoutH commented Nov 17, 2024

Thanks for your PR!

Ensure agent is not giving money to itself

Great catch! I remember fixing this bug somewhere sometime, interesting how it keeps appearing.

@quaquel can we add a method/keyword to exclude yourself from the “get cell contents” and other similar functions?

I don’t agree with the step being merged into a single agent_act. It changes the activation order. Previously, all agents first moved, and then all gave money. Now each agent moves, gives money, and then the next agent etc. Can you motivate why you made that change?

@quaquel
Copy link
Member

quaquel commented Nov 18, 2024

@quaquel can we add a method/keyword to exclude yourself from the “get cell contents” and other similar functions?

This is still using old style spaces, not new style spaces, so yes you can do it. However, we want to deprecate these for 3.1, so I don't see the point. In new-style spaces, it is a lot harder to fix this, since agents is an attribute not a method on both Cell and CellCollection.

@Spartan-71
Copy link
Contributor Author

Spartan-71 commented Nov 18, 2024

I don’t agree with the step being merged into a single agent_act. It changes the activation order. Previously, all agents first moved, and then all gave money. Now each agent moves, gives money, and then the next agent etc. Can you motivate why you made that change?

@EwoutH In the "Adding Space" -> "Moving Mesa" section, there is already a function named step in the MoneyAgent class. However, in the subsequent code snippet, this function seems to disappear. Instead, the move and give_money functions are directly called within the step function of the MoneyModel class.

image

image

While in "Collecting Data" Section the agent_act function is mentioned !

image

Ref: https://mesa.readthedocs.io/stable/tutorials/intro_tutorial.html

@EwoutH
Copy link
Member

EwoutH commented Nov 18, 2024

@tpike3 you last looked into the tutorial, do you know what the intended behavior was?

@Spartan-71
Copy link
Contributor Author

Hey @EwoutH and @tpike3! If there’s anything you’d like to change, just let me know. 😊

@EwoutH
Copy link
Member

EwoutH commented Nov 21, 2024

In the "Adding Space" -> "Moving Mesa" section, there is already a function named step in the MoneyAgent class. However, in the subsequent code snippet, this function seems to disappear. Instead, the move and give_money functions are directly called within the step function of the MoneyModel class.

I think the old step method can be removed, in favor of the new move and give_money implementation.

@tpike3
Copy link
Member

tpike3 commented Nov 23, 2024

@Spartan-71 and @EwoutH Sorry for the delay, if I remember correctly I believe the intent was to show users that there is multiple ways to code it. So I think we have two options

1 - Explicitly call out why they are different in the write up and comments
2 - Make it consistent where the agent function care called form the model step

I dont think we need to belabor this, just need to pick one

Thank you @Spartan-71 for doing this!

@Spartan-71
Copy link
Contributor Author

@tpike3 I chose to go ahead with the 2nd option.

@tpike3
Copy link
Member

tpike3 commented Nov 25, 2024

@tpike3 I chose to go ahead with the 2nd option.

Thanks @Spartan-71!

@tpike3 tpike3 merged commit ca53a86 into projectmesa:main Nov 25, 2024
11 checks passed
@EwoutH
Copy link
Member

EwoutH commented Nov 25, 2024

Thanks for your effort, and congratulations on getting the PR in!

Is this worth backporting to the 3.0.x maintenance branch?

@EwoutH
Copy link
Member

EwoutH commented Nov 25, 2024

@tpike3 minor thing, but could you squash such PRs when merging them?

IMG_0162

@EwoutH EwoutH added the docs Release notes label label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants