Repository: projectmesa/mesa

Issue:
Feature Request: Agent DataCollection Can't Handle Different Attributes in ActivationByType 
**Describe the Feature**
Receive attribute error when running an agent reporter through datacollector when using RandomActivationByType 

**Desired behavior**
I can collect against different attributes based on agent type

**To Reproduce**
Create a model with different agent types and attributes and try to collect against that agent attribute

**Additional context**
Feature would need to update at line 170 of datacollection.py `agent_records = map(get_reports, model.schedule.agents)`


Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) Report
Patch coverage: **`89.47`**% and project coverage change: **`+0.06`** :tada:
> Comparison is base [(`e3af2a5`)](https://app.codecov.io/gh/projectmesa/mesa/commit/e3af2a508080d343eb44f07c5fe5cc9f487a19aa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) 81.49% compared to head [(`f6abceb`)](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) 81.56%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1702      +/-   ##
==========================================
+ Coverage   81.49%   81.56%   +0.06%     
==========================================
  Files          18       18              
  Lines        1405     1421      +16     
  Branches      273      246      -27     
==========================================
+ Hits         1145     1159      +14     
- Misses        214      215       +1     
- Partials       46       47       +1     
```


| [Impacted Files](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) | Coverage Δ | |
|---|---|---|
| [mesa/datacollection.py](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa#diff-bWVzYS9kYXRhY29sbGVjdGlvbi5weQ==) | `89.79% <84.61%> (-0.80%)` | :arrow_down: |
| [mesa/\_\_init\_\_.py](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa#diff-bWVzYS9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | |
| [mesa/model.py](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa#diff-bWVzYS9tb2RlbC5weQ==) | `100.00% <100.00%> (ø)` | |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/projectmesa/mesa/pull/1702?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).

tpike3: I would say it should be part of datacollector as it is a fundamental feature. `DataCollectorWithoutNone` is not intuitively obvious what you can use it for.  
rht: One reason that I separated it was because dropping `None`'s by default, even though it may be intuitive in a lot of cases, is a hidden implicit behavior. Unless you meant to have an explicit flag that enables/disables it.

The naming `DataCollectorWithoutNone` could be improved to signify what it is used for.
jackiekazil: I am trying to wrap my head around this and #1701. From my understanding this would succeed #1701?
rht: This supersedes #1701 and #1481. From the user perspective, if they want to collect `val3` which is available in 1 agent class, but not the rest, then `DataCollectorWithoutNone` will automatically exclude results from other agent classes, because `getattr` returns `None` anyway. As such, it is equivalent to getting`val3` from just the agents you want, and not the rest.

This implementation is also versatile enough that it enables another use case: say if the user wants such that the agent record is not collected all the time, they can write it in their conditional, e.g. collect record only for every 10th time step.
rht: There is no change in the declarative UI, it's still the same `agent_reporters={"value": "val", "value2": "val2"}`. It's just that `None` values are dropped.
Corvince: Isn't [https://github.com/projectmesa/mesa/pull/1701#issuecomment-1555931743](concern) still valid? This doesn't seem to handle the case where attributes don't exist in all agents.

Also in Python None is often a very valid value and it seems kind of strange to always exclude it from data collection 
Corvince: 
> No, it's not a problem. When the attribute doesn't exist in all agents, then the reporter is an empty tuple. Also, no `attrgetter` is involved here.

Yes, sorry, you are correct I was misreading the code. 


> If you don't want to exclude `None`, you can use vanilla `DataCollector`. `None` is not a value that you can plot.

No I cannot use vanilla DataCollector if I want to collect data from multiple agents. This is the reason #1701 exists. 
Also Plotting is not the only reason for data collecting. 

> #1701 has another problem that it requires more RAM that sometimes would cause models to not run in a Binder/Colab environment. The [Sugarscape with trading model](https://github.com/projectmesa/mesa-examples/blob/3612975463c979907de0799480f052ab16931b2d/examples/sugarscape_g1mt/sugarscape_g1mt/model.py#L203-L207) is one example of this situation.


I haven't dug into the details of #1701, but why would it's memory footprint be higher? It should only collect the attributes specified, same as this PR (I know this one also removes any other None values, but this should hardly matter).
If the memory footprint of #1701 is higher that should be an implementation detail that could be changed.

I share your concerns about code complexity though, but I think #1701 could be improved in that respect. 
Corvince: I think what we are actually missing is an interface to the underlying _agent_records. Currently the only public Api to that is getting the data frame out. 

Then this whole PR could be solved in userland by using 

        agent_records_without_none = (r for r in data_collector._agent_records if r is not None)

(Kind of what is done in sugarsacpe example) 

But no one wants to mess with private variables.
rht: > No I cannot use vanilla DataCollector if I want to collect data from multiple agents.

Can you explain this situation? Under which model would you want `None` value and at the same time you want to collect attributes of specific agents?

> It should only collect the attributes specified

#1701's DF is much more complex, and hence it consumes more memory than the temporary workaround @tpike3 did in the Sugarscape with trading, which was already having not enough memory problem, before he manually removed the unwanted values from the internal attributes.

>     agent_records_without_none = (r for r in data_collector._agent_records if r is not None)

This requires a 2nd loop. This PR (#1702) scans through the entire agents, entire agent reporter in 1 loop.

You should consider this PR to be a multipurpose tool, not just for agent-specific attributes. Just as in a DF, there are various ways to interpret and deal with N/A values, your interpretation of `None` could vary depending on the problem in hand, as described in 2nd paragraph of https://github.com/projectmesa/mesa/pull/1702#issuecomment-1556550718.
tpike3: > I think what we are actually missing is an interface to the underlying _agent_records. Currently the only public Api to that is getting the data frame out.
> 
> Then this whole PR could be solved in userland by using
> 
> ```
>     agent_records_without_none = (r for r in data_collector._agent_records if r is not None)
> ```
> 
> (Kind of what is done in sugarsacpe example)
> 
> But no one wants to mess with private variables.
> 
> But again this is different from #1701



> I think what we are actually missing is an interface to the underlying _agent_records. Currently the only public Api to that is getting the data frame out.
> 
> Then this whole PR could be solved in userland by using
> 
> ```
>     agent_records_without_none = (r for r in data_collector._agent_records if r is not None)
> ```
> 
> (Kind of what is done in sugarsacpe example)
> 
> But no one wants to mess with private variables.
> 
> But again this is different from #1701

I am fascinated by this problem as it represents an interesting development and user dynamic. So my view (at least right now until the discussion changes it). 

Overview of #1701 and #1702:
- #1701  explicitly  solves the random activation by type data collection issue and just from a contributor stand out was very cool as it was submitted with test and everything all in, 1st time (That was great @GiveMeMoreData)
- #1702 implicitly solves the random activation by type datacollection issue, but also allows for other datacollector manipulations like only collecting an attribute of an agent if it reaches a threshold. The challenge is for users this is not intuitively obvious

My suggestion: 
- **This change should not be a separate class but just integrated into DataCollector as I believe it fundamentally improves the functionality of datacollector.**  
- I think removing the tuples with `None` should be the default as one somewhat reoccurring user issue is running out memory and anecdotally, even when I ran sugarscape_g1mt on my local machine any effective parameter sweep while not removing the sugar and spice `None` rows crush my RAM, but there should be an option to keep `None` in. 
- As this isn't obvious that this solves the datacollector by type issue, then I will update lessons 19 and 20 in the tutorial and make a gist to reference in the ReadtheDocs so people know how to use datacollector to collect agent attributes. 

Let me know what you think. 

rht: I suppose the 2 main contenders are #1701 and #1702? Should we close #1481, given that it is most complex for the user to use?

Yeah I will merge the 2 classes and add a flag to drop `None` once I have the time window to do it.

> The challenge is for users this is not intuitively obvious

This can be remedied by 1. documenting in the useful code snippets section, 2. examples, in particular the trading Sugarscape. I can do point 1 in a separate PR afterward.
rht: > I think removing the tuples with None should be the default  ...

I'm concerned that this is an implicit behavior.
rht: I have merged the 2 classes into 1. The net diff of this PR is now smaller.
tpike3: > > I think removing the tuples with None should be the default  ...
> 
> I'm concerned that this is an implicit behavior.

That's fair, I am good with the way you set it up
Corvince: I am not sure about this one. 

In comparison #1701 provides such a nice API where you can provide an explicit list of attributes you want to collect for each agent. This allows a fine-grained control over what data is collected. 

Whereas with this PR we have to tell users: yeah just write all attributes you want to collect in one list. Don't worry, If you turn on this flag it will rely on the fact that attributes will default to None if they don't exist and remove them. Oh but please don't collect any attributes that might truly be None, those will also be removed. 
I hope this doesn't come off as snark, but it feels like such a strange and unintuitive way to allow multi agent data collection. How is one supposed to figure this out purely from the API?


I also don't think this is needed for memory issues. I think a more logical approach would be to write out/save _agent_records every n steps and flush the _agent_records dictionary. 

So something like
```python
def step():
    ...
    self.dc.collect()
    df = self.dc.get_agent_vars_df()
    df.save(...)
    self._agent_records = {}
```

Then the Mesa API would only require a method to flush the agent records (or add a flag to get_agent_vars()).

But this is another discussion. 

Corvince: So this was my final comment. I won't continue spamming this PR. I am not completely opposed to merging this but I definitely  don't see this as superseding #1701
GiveMeMoreData: @tpike3 fyi @rht  I like the changes you made and I think they almost solve the problem I had in my case. But I belive there is still a functionality missing. 
To my understanding collecting attributes from different agent classes works only in case when they are expressed  in `agent_reporters` as strings e.g. `{"value": "val"}` due to the fact that reportes are then `getattr(_object, name, None)`, and the `None` handles the case when attr is not present in agent class.  But for case `{"value": lambda x: x.val}` which is also allowed in this package, data collection fails as it did before any changes to main branch. 
tpike3: > 

These are great points and I absolutely agree on the API intuitiveness. I put this in the category of so easy its hard. I think we are at a fundamental trade off between code elegance and user understanding. My thought right now is to try and have the best of both worlds, and mitigate the lack of intuitiveness  by putting examples of agents_by_type_collection in the (1) tutorial, (2) putting the Complexity Explorer and (3) making a gist, so users have lots of examples to see how to do it. 

Feel free to provide a counter argument and one way to assess is if we merge and keeping getting questions on data collection by type then we know we need to make explicit. 
tpike3: > @tpike3 fyi @rht I like the changes you made and I think they almost solve the problem I had in my case. But I belive there is still a functionality missing. To my understanding collecting attributes from different agent classes works only in case when they are expressed in `agent_reporters` as strings e.g. `{"value": "val"}` due to the fact that reportes are then `getattr(_object, name, None)`, and the `None` handles the case when attr is not present in agent class. But for case `{"value": lambda x: x.val}` which is also allowed in this package, data collection fails as it did before any changes to main branch.

@GiveMeMoreData ohhh interesting, I am not able to think through this use case. If one is collecting different agent attributes they will be collected with `getattr`. Maybe the sticking point is `getattr` doesn't require the return to be a string, it could be a int, float, subclass etc. However, I may still be missing something, I can't envision the use case where one would need lambda to collect the attribute value as in `{"value": lambda x: x.val}` . 

Let me know

(For the record I think your code is great! It reminds me of when I rewrote batchrunner_MP and then @Corvince seeing what I did created batch_run in like 1/3 the lines of code, so regardless of how this moves forward your code was absolutely instrumental in developing this solution.) 
GiveMeMoreData: @tpike3 I'm not aware of all the use cases for lambda functions, but they are absolutelly allowed by the library and at least now there is no hint that using them in case of differentiable agents will result in AttributeError, which is currently the case. One simple usecase would be to collect the results of a method implemented in agent. I think that we should change code to either handle lambdas without errors or improve documentation to make it clear that in case of differentiable agents data collection is possible only when described as {"value": "val"}. Also thank you for the very  kind words :)
rht: > I also don't think this is needed for memory issues. I think a more logical approach would be to write out/save _agent_records every n steps and flush the _agent_records dictionary.

This still has memory issue where the DF is huge. And the DF is more complex. because you need extra column to tell which agents have the particular reports.

> I'm not aware of all the use cases for lambda functions, but they are absolutelly allowed by the library and at least now there is no hint that using them in case of differentiable agents will result in AttributeError, which is currently the case

The `AttributeError` from `{"value": lambda x: x.val}` is easy to interpret by the user. They would know right away that some agents don't have the attribute. It's not a fundamental problem. I wouldn't drop this PR (#1702) just for this concern.

Besides, I don't see why one would pick `{"value": lambda x: x.val}` over `{"value": "val"}`, as the latter is what you did in #1701.
rht: I'd say the trade-off is that you have an explicit agent-specific API, but complex inner working of the DF, vs a multi-purpose feature that happens to solve #1419, with a simple inner working. At the end of the day, for #1701, I still need to see an example/tutorial in order to know the structure of the agent-specific DF.
Corvince: > > 
> 
> @Corvince  These are great points and I absolutely agree on the API intuitiveness. I put this in the category of so easy its hard. I think we are at a fundamental trade off between code elegance and user understanding. My thought right now is to try and have the best of both worlds, and mitigate the lack of intuitiveness  by putting examples of agents_by_type_collection in the (1) tutorial, (2) putting the Complexity Explorer and (3) making a gist, so users have lots of examples to see how to do it. 
> 
> Feel free to provide a counter argument and one way to assess is if we merge and keeping getting questions on data collection by type then we know we need to make explicit. 

Again, my point is that this is not a suitable solution for multi agent data collection. This thread already identified two limitations: 1) not possible to collect None values and 2) not possible to use lambda/function reporters if they use unavailable attributes. 

Honestly I think multi agent data collection is such a fundamental feature that it will be very strange to have different constraints from collecting data from a single agent type. 

But, and this is a big but, this PR is mergeable as is and as @rht showed has also a general purpose. So it's better to have some immediate solution than to have a perfect solution that will never be build. So I would say feel free to merge.

But this whole thread got me thinking about the purpose of data collection. I have an idea for a fundamentally different approach, but I will post it as a discussion topic in the next few days.


rht: > 1) not possible to collect None values and 2) not possible to use lambda/function reporters if they use unavailable attributes.

Does that mean you can no longer write a model that collects agent-specific attributes if the 2 features you requested are unavailable? Somehow these features matter more than other more pressing issues that have already been said several times. I have already asked an example of such model, but haven't been provided any.
tpike3: @Corvince I am intrigued to see what you are going to come up with, based on @GiveMeMoreData's comments I did start playing around with integrating lambda functions for different agent types and that also started me thinking about the the idea of datacollection that optimizes APIs, computational efficiency and readable code. I however did not have and breakthrough thoughts. 

Regardless, this code is a great improvement and to @EwoutH comments about contributions we got to be better so I am going to merge.  

Patch:
diff --git a/mesa/datacollection.py b/mesa/datacollection.py
index d58133f1..d383f018 100644
--- a/mesa/datacollection.py
+++ b/mesa/datacollection.py
@@ -51,7 +51,13 @@ class DataCollector:
     one and stores the results.
     """
 
-    def __init__(self, model_reporters=None, agent_reporters=None, tables=None):
+    def __init__(
+        self,
+        model_reporters=None,
+        agent_reporters=None,
+        tables=None,
+        exclude_none_values=False,
+    ):
         """Instantiate a DataCollector with lists of model and agent reporters.
         Both model_reporters and agent_reporters accept a dictionary mapping a
         variable name to either an attribute name, or a method.
@@ -74,6 +80,8 @@ class DataCollector:
             model_reporters: Dictionary of reporter names and attributes/funcs
             agent_reporters: Dictionary of reporter names and attributes/funcs.
             tables: Dictionary of table names to lists of column names.
+            exclude_none_values: Boolean of whether to drop records which values
+            are None, in the final result.
 
         Notes:
             If you want to pickle your model you must not use lambda functions.
@@ -97,6 +105,7 @@ class DataCollector:
         self.model_vars = {}
         self._agent_records = {}
         self.tables = {}
+        self.exclude_none_values = exclude_none_values
 
         if model_reporters is not None:
             for name, reporter in model_reporters.items():
@@ -151,7 +160,23 @@ class DataCollector:
     def _record_agents(self, model):
         """Record agents data in a mapping of functions and agents."""
         rep_funcs = self.agent_reporters.values()
+        if self.exclude_none_values:
+            # Drop records which values are None.
+
+            def get_reports(agent):
+                _prefix = (agent.model.schedule.steps, agent.unique_id)
+                reports = (rep(agent) for rep in rep_funcs)
+                reports_without_none = tuple(r for r in reports if r is not None)
+                if len(reports_without_none) == 0:
+                    return None
+                return _prefix + reports_without_none
+
+            agent_records = (get_reports(agent) for agent in model.schedule.agents)
+            agent_records_without_none = (r for r in agent_records if r is not None)
+            return agent_records_without_none
+
         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)
diff --git a/mesa/model.py b/mesa/model.py
index be607266..15374f70 100644
--- a/mesa/model.py
+++ b/mesa/model.py
@@ -66,7 +66,11 @@ class Model:
         self._seed = seed
 
     def initialize_data_collector(
-        self, model_reporters=None, agent_reporters=None, tables=None
+        self,
+        model_reporters=None,
+        agent_reporters=None,
+        tables=None,
+        exclude_none_values=False,
     ) -> None:
         if not hasattr(self, "schedule") or self.schedule is None:
             raise RuntimeError(
@@ -80,6 +84,7 @@ class Model:
             model_reporters=model_reporters,
             agent_reporters=agent_reporters,
             tables=tables,
+            exclude_none_values=exclude_none_values,
         )
         # Collect data for the first time during initialization.
         self.datacollector.collect(self)


Test patch:
diff --git a/tests/test_datacollector.py b/tests/test_datacollector.py
index 7ba72c73..c44c708e 100644
--- a/tests/test_datacollector.py
+++ b/tests/test_datacollector.py
@@ -32,6 +32,14 @@ class MockAgent(Agent):
         self.model.datacollector.add_table_row("Final_Values", row)
 
 
+class DifferentMockAgent(MockAgent):
+    # We define a different MockAgent to test for attributes that are present
+    # only in 1 type of agent, but not the other.
+    def __init__(self, unique_id, model, val=0):
+        super().__init__(unique_id, model, val=val)
+        self.val3 = val + 42
+
+
 class MockModel(Model):
     """
     Minimalistic model for testing purposes.
@@ -39,13 +47,20 @@ class MockModel(Model):
 
     schedule = BaseScheduler(None)
 
-    def __init__(self):
+    def __init__(self, test_exclude_none_values=False):
         self.schedule = BaseScheduler(self)
         self.model_val = 100
 
-        for i in range(10):
-            a = MockAgent(i, self, val=i)
-            self.schedule.add(a)
+        self.n = 10
+        for i in range(self.n):
+            self.schedule.add(MockAgent(i, self, val=i))
+            if test_exclude_none_values:
+                self.schedule.add(DifferentMockAgent(self.n + i, self, val=i))
+        if test_exclude_none_values:
+            # Only DifferentMockAgent has val3.
+            agent_reporters = {"value": lambda a: a.val, "value3": "val3"}
+        else:
+            agent_reporters = {"value": lambda a: a.val, "value2": "val2"}
         self.initialize_data_collector(
             {
                 "total_agents": lambda m: m.schedule.get_agent_count(),
@@ -54,8 +69,9 @@ class MockModel(Model):
                 "model_calc_comp": [self.test_model_calc_comp, [3, 4]],
                 "model_calc_fail": [self.test_model_calc_comp, [12, 0]],
             },
-            {"value": lambda a: a.val, "value2": "val2"},
+            agent_reporters,
             {"Final_Values": ["agent_id", "final_value"]},
+            exclude_none_values=test_exclude_none_values,
         )
 
     def test_model_calc_comp(self, input1, input2):
@@ -195,5 +211,44 @@ class TestDataCollectorInitialization(unittest.TestCase):
         )
 
 
+class TestDataCollectorExcludeNone(unittest.TestCase):
+    def setUp(self):
+        """
+        Create the model and run it a set number of steps.
+        """
+        self.model = MockModel(test_exclude_none_values=True)
+        for i in range(7):
+            if i == 4:
+                self.model.schedule.remove(self.model.schedule._agents[3])
+            self.model.step()
+
+    def test_agent_records(self):
+        """
+        Test agent-level variable collection.
+        """
+        data_collector = self.model.datacollector
+        agent_table = data_collector.get_agent_vars_dataframe()
+
+        assert len(data_collector._agent_records) == 8
+        for step, records in data_collector._agent_records.items():
+            if step < 5:
+                assert len(records) == 20
+            else:
+                assert len(records) == 19
+
+            for values in records:
+                agent_id = values[1]
+                if agent_id < self.model.n:
+                    assert len(values) == 3
+                else:
+                    # Agents with agent_id >= self.model.n are
+                    # DifferentMockAgent, which additionally contains val3.
+                    assert len(values) == 4
+
+        assert "value" in list(agent_table.columns)
+        assert "value2" not in list(agent_table.columns)
+        assert "value3" in list(agent_table.columns)
+
+
 if __name__ == "__main__":
     unittest.main()