EnergySystemsModellingLab / MUSE_OS

Welcome to the MUSE-OS repository

https://muse-os.readthedocs.io/en/latest/

GNU General Public License v3.0

22 stars 9 forks source link

Update demand_share.py _inner_split function to solve reoccuring error message when running MUSE with multiple agents #253

Closed LennartMorlock closed 4 months ago

LennartMorlock commented 6 months ago

Description

This commit includes the minor change to the _inner_split function in the demand_share.py file, described in the related issue.

It solves the error. However, uncertainty about its sensibility remains from a modelling perspective.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s.

[ ] New feature (non-breaking change which adds functionality)
[ ] Optimization (non-breaking, back-end change that speeds up the code)
[ ] Bug fix (non-breaking change which fixes an issue)
[ ] Breaking change (whatever its nature)

Key checklist

[ ] All tests pass: $ python -m pytest
[ ] The documentation builds and looks OK: $ python -m sphinx -b html docs docs/build

Further checks

[ ] Code is commented, particularly in hard-to-understand areas
[ ] Tests added that prove fix is effective or that feature works

martinstringer commented 6 months ago

Hi Lennart, thank you from all of us for proposing this fix. If time allows, would you be able to also share your understanding of what your proposed changes are doing? If this could be added directly as comments to the new lines of code, even better.

alexdewar commented 4 months ago

Closing as this is stale and @LennartMorlock seems unresponsive.

LennartMorlock commented 4 months ago

Sorry for not responding earlier. I created a much-simplified model (attached) that causes the same error aimed to be solved with this pull request. I added it to the related issue as well.

simple_model_error.zip

alexdewar commented 4 months ago

Hi @LennartMorlock. Thanks for your reply. I was able to replicate the issue with the config files you attached.

I'm not sure what this function is supposed to be doing either tbh! It's a little opaque. I'm tagging my colleague @dalonsoa who might have a better idea.

dalonsoa commented 4 months ago

Not much to add to what is said in the docstring. The demand of any commodity needs to be split between the agents so they can use their share of the demand to invest in new technologies. Now, why there's an unassigned, why is calculated that way and its relationship with total, I don't really know.

alexdewar commented 4 months ago

Okey doke. I'll have a play with it a bit more to see if I can understand it better, but provided that it passes tests, would you be happy if this is merged?

dalonsoa commented 4 months ago

After updating the branch, yes, I think that's fine.

LennartMorlock commented 4 months ago

Hi @alexdewar, yeah, the changes seem good to me. I still get the same results and the code definitely looks cleaner.

alexdewar commented 4 months ago

Great, thanks :smile: