feat: Implement exclude_none_values in DataCollector

[rht]

Fixes #1419.

Came up with this solution while reviewing #1701; thank you @GiveMeMoreData. This has 2 benefits:

• Simpler underlying code, and hence simpler mental model of how it works under the hood and simpler to document.
• Only 1 loop through the entire agents is needed, and hence, should be more performant compared to solutions that additionally loop through specific agents separately, or defining multiple data collectors (as in Fix #1419. DataCollector accepts an arbitrary schedule at creation (d… #1481).

In this case, if you create a DataFrame out of the agent records, the agents that don't have the said attribute will have nan values in the column (if it the column is typed as a number).

One point to discuss is whether I should merge DataCollectorWithoutNone into DataCollector, and have a flag in DataCollector to enable/disable of the exclusion of None values.

@woolgathering what do you think of this solution for your model?

[codecov]

Codecov Report

Patch coverage: 84.61% and project coverage change: +0.01 🎉

Comparison is base (6f08b07) 81.45% compared to head (d4e173a) 81.47%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1702      +/-   ##
==========================================
+ Coverage   81.45%   81.47%   +0.01%     
==========================================
  Files          18       18              
  Lines        1402     1414      +12     
  Branches      272      278       +6     
==========================================
+ Hits         1142     1152      +10     
- Misses        214      215       +1     
- Partials       46       47       +1     

Impacted Files	Coverage Δ
mesa/model.py	100.00% <ø> (ø)
mesa/datacollection.py	89.36% <84.61%> (-0.89%)	⬇️

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

[tpike3]

Just too unpack this a little.. if the agent does not have the attribute, None is returned and and there will be no tuple and then no row in the DataFrame This prevents the issues we saw in Sugarscape with Traders where the Sugar and Spice information was retained as None and due to the number of sugar and spice agents this created memory issues.

However, if the attribute is typed as a number then it will be collected as a NaN. But based on the dynamics of this should have less memory issues.

[rht]

Just too unpack this a little.. if the agent does not have the attribute, None is returned and and there will be no tuple and then no row in the DataFrame This prevents the issues we saw in Sugarscape with Traders where the Sugar and Spice information was retained as None and due to the number of sugar and spice agents this created memory issues.

Yes, attributes which values are retrieved as None are excluded.

However, if the attribute is typed as a number then it will be collected as a NaN. But based on the dynamics of this should have less memory issues.

This is not true, as long as you define the function to return None when appropriate. The NaN values only appear when the report get converted to a DataFrame. This is because a DF is constrained to be a table, so a N/A value for a number is NaN.

[tpike3]

Just too unpack this a little.. if the agent does not have the attribute, None is returned and and there will be no tuple and then no row in the DataFrame This prevents the issues we saw in Sugarscape with Traders where the Sugar and Spice information was retained as None and due to the number of sugar and spice agents this created memory issues.

Yes, attributes which values are retrieved as None are excluded.

However, if the attribute is typed as a number then it will be collected as a NaN. But based on the dynamics of this should have less memory issues.

This is not true, as long as you define the function to return None when appropriate. The NaN values only appear when the report get converted to a DataFrame. This is because a DF is constrained to be a table, so a N/A value for a number is NaN.

Ahhh got it, very cool

[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?

Can you demonstrate what this would look like from a user perspective?

[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 gettingval3 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, by having the record function to return None when not needed.

Edit: clarify 2nd paragraph

[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 @GiveMeMoreData 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

[rht]

Isn't @GiveMeMoreData #1701 (comment) still valid? This doesn't seem to handle the case where attributes don't exist in all agents.

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.

Also in Python None is often a very valid value and it seems kind of strange to always exclude it from data collection

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

#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 is one example of this situation.

[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 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.

But again this is different from #1701

[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 #1702 (comment).

[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:

• Collecting data from multiple differentiable classes of agents #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)
• feat: Implement exclude_none_values in DataCollector #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

[tpike3]

LGTM

• Due to the high volume of discussion on this, Collecting data from multiple differentiable classes of agents #1701, and Fix #1419. DataCollector accepts an arbitrary schedule at creation (d… #1481, I will merge in 48 hours if no further comments.
• @Corvince , @GiveMeMoreData, @woolgathering, @EwoutH please provide any thoughts or concerns you may have.

[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

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]

@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.

[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.

[EwoutH]

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.

This this issue ever get resolved? Especially in the case of:

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.

Since I’m touching the DataCollector in #1838 again, I’m considering if it needs to be tested with multiple agent classes, each with multiple types of agents reporter syntaxes (strings, lambda, etc.)

[jacob-thrackle]

Following again (this is @woolgathering) since I've returned from tooling-land to get back into simulations.

In my mind, it would be nice to test with multiple agent classes and reporter syntaxes so cover all the bases. My feeling is that if we're mucking in it now we might as well cover as much as is reasonable. I would expect some of my use cases to run into those problems.

[rht]

There is a solution proposed in #1813 (comment), but haven't been implemented.

[EwoutH]

Is there consensus about that solution being the right way to go (among @tpike3 @jackiekazil @Corvince @GiveMeMoreData)? If so, would you be willing to open a PR?

I feel we have talked so much about configuring the DataCollector for multiple agent types, we should figure it out once and for all (looked it up, literally since early 2017, nearly 7 years ago, this discussion has been ongoing on and of).