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

Boltzmann wealth model: Use the new Mesa 2.2 API & some refactors #79

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rht
Copy link
Contributor

@rht rht commented Jan 7, 2024

@rht rht changed the title Boltzmann wealth model: Use the new model.agents API Boltzmann wealth model: Use the new model.agents API & some refactors Jan 7, 2024
@rht rht changed the title Boltzmann wealth model: Use the new model.agents API & some refactors Boltzmann wealth model: Use the new Mesa 2.2 API & some refactors Feb 15, 2024
@rht
Copy link
Contributor Author

rht commented Feb 15, 2024

This is ready for review.

@rht rht requested a review from EwoutH February 15, 2024 23:04
Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One thing I would like to have changed, otherwise looks good!

Approving assuming the change below is made.

if len(cellmates) > 0:
other = self.random.choice(cellmates)
other.wealth += 1
self.wealth -= 1

def step(self):
self.move()
if self.wealth > 0:
self.give_money()
self.give_money()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the old check here better, it made it explicit that only money was given when there was any to give.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about maybe_give_money?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah renaming the function would also work (consider_giving_money maybe), but for explicitness I liked the old structure better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning is different. The check gives a constraint to the give_money method that prevents negative wealth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Corvince @tpike3 @quaquel what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just curious why you said the name choice is based on Haskells Maybe monad. If it's just about maybe doing something, then the dictionary definition of maybe would suffice. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already said this

If you are concerned about the connotation with the monad, then there is this usage in Golang.

It's just my first encounter on the usage of the term "maybe" in naming in the context of programming.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I have already said that I am not concerned but just curious 😅

I have no idea what you want to tell me, but we should probably just leave this conversation. It's not important.

Copy link
Member

@EwoutH EwoutH Feb 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think the older version was better, because more explicit.

I agree. It's more explicit and more readable. These are user targeted examples, that show behavior. Not some mission critical application. Let's leave it the old way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted.

@rht rht force-pushed the agentset branch 2 times, most recently from d55d144 to 6256ce4 Compare February 17, 2024 23:51
@tpike3
Copy link
Member

tpike3 commented Feb 22, 2024

The tests are failing is this because the tests are now wrong? (Apologies for the delay)

@rht
Copy link
Contributor Author

rht commented Feb 22, 2024

Fixed in projectmesa/mesa#2050.

@EwoutH
Copy link
Member

EwoutH commented Feb 22, 2024

Looks good to go!

@quaquel
Copy link
Member

quaquel commented Feb 22, 2024

I agree, this can be merged.

@rht
Copy link
Contributor Author

rht commented Feb 22, 2024

If this is merged, anyone trying to run the example from the latest stable release will encounter an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants