Simplify the agents module, and additional changes #507
Conversation
tsmbland
changed the title
tsmbland
changed the title
| # Calculates the demand divided by the number of assets times the number of agents | ||
| # if the demand is bigger than zero and the total demand assigned with the "method" | ||
| # function is zero. | ||
| unassigned = (demand / (len(shares) * len(summed_shares))).where( |
There was a problem hiding this comment.
Reeeeeally bad idea to call len on a DataArray (summed_shares), as this will just take the length of the first dimension, which isn't guaranteed to be the dimension you're after ("asset" in this case), unless you're really careful about it. In this case we get away with it, but this has bitten me on another PR and took me a long time to figure out.
Also, I still don't really understand what this function is doing. Why is it multiplying the number of agents by the number of assets? What does totals represent?
dalonsoa
left a comment
dalonsoa
left a comment
There was a problem hiding this comment.
I've made a few comments and suggestions here and there, but everything seems to make sense to me.
It is illustrative of the state of MUSE that after all the code you have removed or changed, the tests are largely the same.
| ) | ||
|
|
||
|
|
||
| class Agent(AbstractAgent): |
There was a problem hiding this comment.
I'm pretty sure this is some sort of legacy feature not really used except, as you mention, by trade. But that's a very obscure sector that I've no idea how it works or what it does.
|
|
||
| # Skip forward if the search space is empty | ||
| if any(u == 0 for u in search_space.shape): | ||
| getLogger(__name__).critical("Search space is empty") |
There was a problem hiding this comment.
Should the simulation continue after this sort of critical message? If it can, then it might not be that critical and just a warning is enough.
There was a problem hiding this comment.
In theory it can continue, the agent just won't perform any investments for that year. If this happens, then it probably means there's a mistake with the way the model is defined, so this is something the user should definitely pay attention to. In any case, I'm a bit concerned that a lot a important messages are getting ignored, so have suggested these get outputted to a file (#498)
| commodity=technologies.commodity, region=technologies.region | ||
| ).interp(year=[2020, 2025]) | ||
| assert all(agent.year == 2020 for agent in agents) | ||
| result = subsector.aggregate_lp(technologies, market) |
There was a problem hiding this comment.
Why was this returning something different that None? I think this test only makes sense if aggregate_lp returns something different than None, which we have seen cannot happen. And if it can happen, as this test was demonstrating, under what conditions? Are we removing functionality that is actually useful (not necessarily in use)?
There was a problem hiding this comment.
When using the Agent class, aggregate_lp would return the search space and constraints, and then these would be used to perform investments withing Subsector.invest. Conversely, the InvestingAgent performs investments within its own next method. In theory the net result should be the same, so I can't see any reason to use Agent over InvestingAgent. I'm guessing it's just a legacy thing that was never tidied up
This PR combines a few small changes which I made while looking through the code and trying to understand how everything works. A lot of the changes are just to make the code more readable (e.g. adding inline comments), but there are a few changes to the actual code as well:
Remove some default values for function arguments. Specifically things like the year and forecast length. I think generally it's better to be explicit about these things, and I couldn't really see any reason you'd ever want these to rely on the defaults (apart from maybe making the tests simpler)
Remove
agents.factories.factory(never used)Remove
demand_share.new_demand(never used)Remove the
protectedargument fromcliff_retirement_profile. The code was needlessly complex, and the equivalent can be done by clipping the inputRemoving some unnecessary interpolation operations from
Sector.nextTidy up the
AgentandInvestingAgentclasses. I didn't like how a lot of the code for theInvestingAgentwas wrapped up in thenextmethod of theAgentclass. I've tidied things up so that all the code related to investments is in theInvestingAgentclassaggregate_lpno longer returns anything. It's very difficult to tell when this function is supposed to output something, and what that output was supposed to be used for. Looking through the code, it appears this would only ever be used when using a noninvesting agent (Agentclass rather thanInvestingAgent), which is used when specifying the agent type as "agent", "default" or "standard". However, the only options given for this parameter in the documentation are "new" and "retrofit" (which both use theInvestingAgentclass), so I can only assume these are legacy options, and this pathway isn't required any more (although maybe I'm missing something?). In which case, we can probably simplify the agents module even more that I've done here, bit I'll leave it for now. For what it's worth, the "trade" example model does use the "default" agent type, but isn't affected by these changesThis doesn't address any particular issues, but I'm increasingly finding that lack of readability is a barrier for bug-fixing and development, so I think any attempts to address this will be helpful in the long run, even if we end up breaking backwards-compatibility or undocumented features.
Probably should have broken this into multiple PRs, but I didn't really go into this with any sort of plan, so this is just how its ended up.