Conversation
ee68189 to
5b40711
Compare
96f5c9d to
db42c16
Compare
This reverts commit a66388d.
for more information, see https://pre-commit.ci
3b5dd98 to
a840748
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR splits demand across subsectors and updates the associated tests, constraints, and investments functions to handle commodities explicitly. Key changes include:
- Removal of the obsolete lp_costs test and update of test fixtures to include a 'commodities' parameter.
- Adjustments in constraint and cost formulations to separate capacity and production decision variables.
- Modifications in the subsector and agent modules to filter and pass commodity‐specific data.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_trade.py | Removed the obsolete lp_costs test to streamline trade model testing. |
| tests/test_constraints.py | Updated fixtures and assertions to include commodity-based constraints. |
| tests/test_agents.py | Marked retrofit agent test as xfail and imported additional markers. |
| src/muse/sectors/subsector.py | Modified demand splitting to select commodity-specific consumption data. |
| src/muse/investments.py | Updated function calls by passing parameters as keywords for clarity. |
| src/muse/constraints.py | Refactored LP cost calculations and constraint transformations with new renaming rules. |
| src/muse/agents/agent.py | Filtered technologies based on the search space and passed commodities to constraints. |
Comments suppressed due to low confidence (3)
src/muse/constraints.py:194
- Directly asserting that 'constraint.b' is not scalar may cause unexpected failures if scalar constraints are valid. Consider explicitly checking the number of dimensions and handling the scalar case appropriately.
assert not constraint.b.dims == ()
src/muse/constraints.py:795
- Ensure that 'pandas' is imported (e.g., 'import pandas as pd') since 'pd.Index' is used, to prevent a runtime error.
production_costs["timeslice"] = pd.Index(production_costs.get_index("timeslice"), tupleize_cols=False)
src/muse/constraints.py:1341
- [nitpick] Using string slicing for dimension renaming (str(k)[2:-1]) may be fragile if dimension names change unexpectedly. Consider using a more explicit and robust renaming strategy.
return result.rename({k: str(k)[2:-1] for k in result.dims})
dalonsoa
left a comment
There was a problem hiding this comment.
This is a quite meaty PRs, so it is going to taka a while. I've managed to review just the constrains, for now, with some comments and suggestions.
| def to_muse(x: np.ndarray) -> xr.Dataset: | ||
| return ScipyAdapter._back_to_muse(x, capacities.costs, productions.costs) | ||
| return ScipyAdapter._back_to_muse( | ||
| x, | ||
| capacity_template=capacities.costs, | ||
| production_template=productions.costs, | ||
| ) |
There was a problem hiding this comment.
I always thought this was a very odd design pattern. Why not simply storing the templates in attributes of ScipyAdapter and making the relevant methods like back_to_muse not static? It will be way simpler to follow.
All in all, and considering the size of this file, I think it will make sense to extract this as in its own module and make most of these static methods just plain functions in there. They way they are used here feels like the wrong way of using static methods, specially when you are calling them within the class itself.
There was a problem hiding this comment.
I agree, it's odd. I think I'll leave it as it is for now and have a play around in another PR, otherwise this PR will become very difficult to review
dalonsoa
left a comment
There was a problem hiding this comment.
Nice. I think now things look very good and are easier to understand. Together with the tutorial, I think this is a great step forward.
Description
MUSE has a subsectors feature, which has been largely neglected up until now (most sectors have a single "all" subsector), but come into focus recently as researchers want to split up large sectors into multiple subsectors serving distinct commodity demands (e.g. a residential sector that serves both heating and lighting demand using different technologies). There are two reasons why this is useful:
However, things are currently not working. If you try to split up a sector into multiple subsectors you will end up with overinvestment by a factor equal to the number of subsectors, and it will take longer to run (again, by approximately a factor equal to the number of subsectors). Hmm, suspicious.
The reason is that each subsector is getting passed the full set of commodity demands and technologies for the whole sector, and performs a full optimization problem aiming to meet the full commodity demands of the sector with the full set of technologies. Repeating this for each subsector, then adding subsector investments together, you get overinvestment (and a long running time).
We need to split the commodity demands and technologies between the subsectors, so that then each subsector solves a reduced problem.
Main changes to the code:
subsectors.py). This is based onself.commodities, which was already in place but not being used. There are already checks in place to ensure that subsectors have non-overlapping commodities.agent.py). As of Limit search space to relevant technologies #687, the search space will only contain relevant technologies that can actually service the demands selected for above, so we can shrink the technologies dataset based on this. (I don't actually think this is necessary as the search space will already filter irrelevant technologies out of the optimization problem, but still worth doing for safety)lp_coststo take a list of commodities as an argument so it creates decision variables only for these commodities. This function was over-complicated anyway so I've simplified itAnd voila. Each subsector now solves a reduced problem to service a subset of commodities in the sector. Since subsector commodities are non-overlapping (and this is enforced), we shouldn't get any doubling up of investment.
If any of this is unclear (probably), I've created a tutorial in #690 which is based off this branch. This shows that things are working nicely as expected.
Other changes:
constraintsmodule as I went through trying to understand what each function was doing. There are a lot of diffs, but the only function with major changes to the code islp_costs(and a couple of the constraints, see next point)demandandtechnologies, but since this filtering is now done outside of the constraints I've removed thisconstraintsmodule (ScipyAdapterandlp_constraint_matrix). I banged my head against a wall for ages trying to get these to work and eventually gave up and decided I have better things to do with my time. I didn't find them particularly helpful or well written anyway, and, quite frankly, if they can start failing despite me doing nothing major to the functions in question, then they're not good tests anyway. Hopefully the addition of inline comments to the code makes up for thistest_run_retro_agentstarted failing. I don't know why, but I'm not going to invest time into fixing it as retrofit agents are on their way out (planning to add a deprecation warning in the next release). Thedefault_retromodel still works though, so don't think it's a major breakagetest_trade. Similar to above, the trade feature is a massive headache and I'm not going to waste time trying to fix it, especially since the trade results are currently nonsense anyway. Thetrademodel still runs, however.test_constraints, mostly to tidy up and get everything passing again (like is often the case, fixing the failing tests took 10x longer that fixing the bug itself!)_unified_datasettooktechnologiesas an argument, but this wasn't used so I've removed itFixes #684
Provides a solution to #389, since the sector in question can be split up into multiple subsectors. However, I'll leave that issue open for now as a reminder that MUSE is still incredibly wasteful with memory.
Type of change
Key checklist
$ python -m pytest$ python -m sphinx -b html docs docs/buildFurther checks