Simplify the agents module, and additional changes

[tsmbland]

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 protected argument from cliff_retirement_profile. The code was needlessly complex, and the equivalent can be done by clipping the input

• Removing some unnecessary interpolation operations from Sector.next

• Tidy up the Agent and InvestingAgent classes. I didn't like how a lot of the code for the InvestingAgent was wrapped up in the next method of the Agent class. I've tidied things up so that all the code related to investments is in the InvestingAgent class

• aggregate_lp no 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 (Agent class rather than InvestingAgent), 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 the InvestingAgent class), 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 changes

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

[tsmbland]

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]

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.

[dalonsoa]

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.

[dalonsoa]

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.

[tsmbland]

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)

[dalonsoa]

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

[tsmbland]

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