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

Collecting data from multiple differentiable classes of agents #1701

Closed
wants to merge 4 commits into from

Conversation

GiveMeMoreData
Copy link

Curent version of mesa did allow for capturing data from multiple classes, but collected variables had to be present in all agents, which for large number of agent classes and variables, results in messy and unnecessary code. Modification present in this PR allows to write collection rules for specific class of agents. This changes the current code from

class MockAgent(Agent):
    def __init__(self, unique_id, model, val=0):
        super().__init__(unique_id, model)
        self.val = val
        self.val2 = None # used only in DifferentMockAgent


class DifferentMockAgent(Agent):
    def __init__(self, unique_id, model, val=0):
        super().__init__(unique_id, model)
        self.val = None # used only in MockAgent
        self.val2 = val

agent_reporters = {"value": "val", "value2": "val2"}

to

class MockAgent(Agent):
    def __init__(self, unique_id, model, val=0):
        super().__init__(unique_id, model)
        self.val = val


class DifferentMockAgent(Agent):
    def __init__(self, unique_id, model, val=0):
        super().__init__(unique_id, model)
        self.val2 = val

agent_reporters = { MockAgent: {"value": "val"}, DifferentMockAgent: {"value2": "val2"}}

Agent specific variables are collected in separate dictionary, which can later be retrieved with get_agent_specific_vars_dataframe as dataframe copyting the bahaviour of get_agent_vars_dataframe, but with addition of one index column containg agent class info.

        agent_vars = data_collector.get_agent_vars_dataframe()
        specific_agent_vars = data_collector.get_agent_specific_vars_dataframe()

@GiveMeMoreData
Copy link
Author

pre-commit.ci autofix

@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Patch coverage: 95.65% and project coverage change: +0.42 🎉

Comparison is base (e3af2a5) 81.49% compared to head (6c94ef0) 81.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1701      +/-   ##
==========================================
+ Coverage   81.49%   81.91%   +0.42%     
==========================================
  Files          18       18              
  Lines        1405     1449      +44     
  Branches      273      289      +16     
==========================================
+ Hits         1145     1187      +42     
- Misses        214      215       +1     
- Partials       46       47       +1     
Impacted Files Coverage Δ
mesa/datacollection.py 91.73% <94.73%> (+1.14%) ⬆️
mesa/time.py 91.66% <100.00%> (+0.66%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rht
Copy link
Contributor

rht commented May 19, 2023

This gives me an idea for a much simpler solution. Basically, modify

agent_records = map(get_reports, model.schedule.agents)

to

agent_records = []
for agent in model.schedule.agents:
    record = get_reports(agent)
    record = tuple(r for r in record if r is not None)
    if len(record) == 0:
         continue
    agent_records.append(record)

You do agent_reporters={"value": "val", "value2": "val2"}. @tpike3 what do you think of this solution for trading Sugarscape?
Edit: Added check for zero length record.

@rht
Copy link
Contributor

rht commented May 19, 2023

In my solution, you can have multiple different agent classes with the same attribute name to be collected together.

@GiveMeMoreData
Copy link
Author

In my solution, you can have multiple different agent classes with the same attribute name to be collected together.

Yea, but what about classes with different attributes, which is the sole reason for this PR? If you see possible changes in overall collection process, thats great, but for simplicity lets focus on the matter in hand.

@tpike3
Copy link
Member

tpike3 commented May 19, 2023

In my solution, you can have multiple different agent classes with the same attribute name to be collected together.

Yea, but what about classes with different attributes, which is the sole reason for this PR? If you see possible changes in overall collection process, thats great, but for simplicity lets focus on the matter in hand.

This is awesome!! This has been a big gap (#1419) and if we do a quick release to PyPI, I can incorporate into the Complexity Explorer tutorial for sugarscape.

To @rht's point there may be a more elegant way to do this, but it has been brought up that Mesa needs to be easier to contribute to (Thank you @EwoutH for your candid feedback) and per open source etiquette we must be polite and positive (this is why @jackiekazil pinged me to get on the code of conduct)

I say we merge and then release Mesa 1.2.2. What do you say @rht?

@rht
Copy link
Contributor

rht commented May 19, 2023

The reason why I said my proposed solution in the first place is because it solves the problem brought up in this PR (classes with different attributes) in a much simpler way. I'd prefer to base on Occam's razor to have the codebase not contain complicated solution if possible.

@rht
Copy link
Contributor

rht commented May 19, 2023

but it has been brought up that Mesa needs to be easier to contribute to (Thank you @EwoutH for your candid feedback) and per open source etiquette we must be polite and positive (this is why @jackiekazil pinged me to get on the code of conduct)

I don't think I have crossed any line regarding with the code of conduct. I was just pointing out a different way to solve the problem.

@rht
Copy link
Contributor

rht commented May 19, 2023

I suppose I need to elaborate on why my solution solves classes with different attributes. With agent_reporters={"value": "val", "value2": "val2"}, if class A has only val, and class B has only val2, they will be collected into value and value2 separately. In another situation, where you want val and val2 to be collected into value, you can do a custom lambda function lambda agent: agent.val if isinstance(agent, A) else agent.val2.

@GiveMeMoreData
Copy link
Author

I suppose I need to elaborate on why my solution solves classes with different attributes. With agent_reporters={"value": "val", "value2": "val2"}, if class A has only val, and class B has only val2, they will be collected into value and value2 separately. In another situation, where you want val and val2 to be collected into value, you can do a custom lambda function lambda agent: agent.val if isinstance(agent, A) else agent.val2.

Okey, I might be missing something, but the get_report created from agent_reporters={"value": "val", "value2": "val2"} is in both cases attrgetter and it returns an AttributeError when you are trying to get an nonexisting attribute of a class.

Even after your proposed change this code still results in an error

from mesa import Agent, Model
from mesa.time import BaseScheduler

class MockAgent(Agent):
    def __init__(self, unique_id, model, val=0):
        super().__init__(unique_id, model)
        self.val = val


class DifferentMockAgent(Agent):
    def __init__(self, unique_id, model, val=0):
        super().__init__(unique_id, model)
        self.val2 = val


class MockModel(Model):
    schedule = BaseScheduler(None)

    def __init__(self):
        self.schedule = BaseScheduler(self)

        self.schedule.add(MockAgent(1, self))
        self.schedule.add(DifferentMockAgent(2, self))
        
        self.initialize_data_collector(
            agent_reporters={"value": "val", "value2": "val2"}
        )
        
    def step(self):
        self.schedule.step()
        self.datacollector.collect(self)
        
model = MockModel() 
model.step()
    
agent_vars = model.datacollector.get_agent_vars_dataframe()

@Corvince
Copy link
Contributor

agent_reporters = { MockAgent: {"value": "val"}, DifferentMockAgent: {"value2": "val2"}}

I just wanted to say that I love the semantics here. Using the agent classes directly as an identifier is just so explicit that it should be obvious for what it does. Awesome

@tpike3
Copy link
Member

tpike3 commented May 22, 2023

@GiveMeMoreData Take a look #1701. I believe this addresses your concerns but let us know.

@Corvince
Copy link
Contributor

Maybe we should revisit this PR after the discussions in #348 . It seems to be rather additive to #1702 rather than superseded by that PR.

@Corvince
Copy link
Contributor

I suppose I need to elaborate on why my solution solves classes with different attributes. With agent_reporters={"value": "val", "value2": "val2"}, if class A has only val, and class B has only val2, they will be collected into value and value2 separately. In another situation, where you want val and val2 to be collected into value, you can do a custom lambda function lambda agent: agent.val if isinstance(agent, A) else agent.val2.

Okey, I might be missing something, but the get_report created from agent_reporters={"value": "val", "value2": "val2"} is in both cases attrgetter and it returns an AttributeError when you are trying to get an nonexisting attribute of a class.

I think maybe it would be best to remove the performance optimization from attrgetter, i.e. the lines

        if all(hasattr(rep, "attribute_name") for rep in rep_funcs):
            # This branch is for performance optimization purpose.
            prefix = ["model.schedule.steps", "unique_id"]
            attributes = [func.attribute_name for func in rep_funcs]
            get_reports = attrgetter(*prefix + attributes)`

I think this would ease multi agent data collection, albeit with None values, which could be removed with the option added in #1701, if we find a way to fix it.

Should i open such a PR?

@rht
Copy link
Contributor

rht commented Sep 18, 2023

Yeah, the performance optimization is not worth the added opaqueness of the code.

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.

4 participants