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

Fix bug in tutorial to calculate MRS with potential trade #33

Merged
merged 1 commit into from
May 5, 2023

Conversation

tpike3
Copy link
Member

@tpike3 tpike3 commented May 1, 2023

  • update maybe_sell_spice() so calculate_MRS() takes potential sugar and spice based on trade
  • update calculate_MRS() with kwargs so it can take potential trade but defaults to agents sugar and spice
    Update increases agent_reporter due to number of trades creating memory issues
  • Update data collection so non-relevant data rmeoved on each step (i.e. sugar and spice agents don't have trade partners)

@rht
Copy link
Contributor

rht commented May 1, 2023

I think we should set sugar and spice to be required arguments, making them explicit, and thus, requires less justification on whether they are correct.

@tpike3
Copy link
Member Author

tpike3 commented May 2, 2023

Made calculate_MRS so it requires sugar and spice. I also increased the number of runs since the memory issue is mitigated.

@@ -187,6 +187,13 @@ def step(self):

# collect model level data
self.datacollector.collect(self)
# Need to remove excess data
# Create local variable to store trade data
Copy link
Contributor

Choose a reason for hiding this comment

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

What about projectmesa/mesa#1419 (comment) ? Which doesn't expose the internal wiring of the DataCollector.

Copy link
Member Author

Choose a reason for hiding this comment

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

My concern with putting it in the model collector is the point of the lesson is to talk about the agent reporter. Showing the internal wiring of datacollector, I think is more consistent with the learning objectives as I show the source code in various lessons.. This leads back to your second comment that we would need to build a class for collecting DataCollectorRABT.

So in I would prefer to show the internal wiring... I don't know.. what do you think?

Copy link
Contributor

@rht rht May 3, 2023

Choose a reason for hiding this comment

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

I suppose it is fine for now as long as there is a comment that says this part is still a work in progress, with a link to #1419.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added

@rht
Copy link
Contributor

rht commented May 4, 2023

Needs Black formatting,

@tpike3
Copy link
Member Author

tpike3 commented May 4, 2023

Needs Black formatting,

Done

@rht
Copy link
Contributor

rht commented May 4, 2023

There are still trailing whitespaces in examples/sugarscape_g1mt/sugarscape_g1mt/model.py.

- update maybe_sell_spice() so calculate_MRS() takes potential sugar and spice based on trade
- update calculate_MRS() with kwargs so it can take potential trade but defaults to agents sugar and spice
Update increases agent_reporter due to number of trades creating memory issues
- Update data collection so non-relevant data rmeoved on each step (i.e. sugar and spice agents don't have trade partners)
@tpike3
Copy link
Member Author

tpike3 commented May 5, 2023

There are still trailing whitespaces in examples/sugarscape_g1mt/sugarscape_g1mt/model.py.

Sorry about that forget to install pre-commit in my mesa-examples.

@rht
Copy link
Contributor

rht commented May 5, 2023

LGTM

@rht rht merged commit 9f3ceb9 into projectmesa:main May 5, 2023
rht pushed a commit to rht/mesa-examples that referenced this pull request May 18, 2023
…a#33)

- update maybe_sell_spice() so calculate_MRS() takes potential sugar and spice based on trade
- update calculate_MRS() with kwargs so it can take potential trade but defaults to agents sugar and spice
Update increases agent_reporter due to number of trades creating memory issues
- Update data collection so non-relevant data rmeoved on each step (i.e. sugar and spice agents don't have trade partners)
rht pushed a commit that referenced this pull request May 19, 2023
- update maybe_sell_spice() so calculate_MRS() takes potential sugar and spice based on trade
- update calculate_MRS() with kwargs so it can take potential trade but defaults to agents sugar and spice
Update increases agent_reporter due to number of trades creating memory issues
- Update data collection so non-relevant data rmeoved on each step (i.e. sugar and spice agents don't have trade partners)
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.

2 participants