Features/update existing pipes - #164
Conversation
nominal_value was deprecated in oemof-solph 0.6
"if q["existing"]" evaluates as True for NaN
No change of functionality.
When the setting "bidirectional_pipes" is False, new heatpipes are created in both directions, to allow potential flows in both directions. Apply same logic to existing pipes.
Assume undefined are non-existing, i.e. new pipes.
- Return existing pipes in the resulting network. Only applies if "existing=1" was used for any input pipe segments. - Detect flow direction of existing pipes (not only new pipes) - Set "flow" as a new attribute of pipes. For new pipes "capacity = flow + losses". For existing pipes "capacity >= flow + losses". Therefore flow is otherwise unknown for existing pipes.
If there are existing pipes in the input data, do not weld/merge/dissolve pipe segments with different capacities, since this removes the information. The new attribute "retain_unique_values" makes sure the selected unique values are preserved.
Use a consistent approach for new and existing pipes instead of using 'size' attribute, which yields inconsistencies for existing pipes and cases with 'bidirectional_pipes'=True.
Include changes to 'simplify' and existing pipes
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I would delegate a new test of a complete solver workflow with cbc to a new PR. That should improve coverage massively. |
p-snft
left a comment
There was a problem hiding this comment.
I have mixed feeling about this. The PR improves functionality, but even explicitly increases technical debt.
I would delegate a new test of a complete solver workflow with cbc to a new PR.
I cannot really blame you, as the architecture of dhnx isn't really modular, so (unit) testing also is an issue. At the moment, I feel like we need a big refactor anyway, but I do not see who has dedicated time for that purpose.
* Add test for investment optimization workflow * Simplify test_process_geometry()
|
I realized cbc was already installed in the workflows, so implementing new tests for the complete optimization process was quite simple, after all. I now included two new tests that cover several different options. Now we just crossed 70% total coverage. I feel like increasing the coverage further is beyond the scope of this PR. |
|
The test coverage does not really say anything, as we don't have unit tests here. It's only that the integration test touches 70 % of the code. (This will not help to find out where in the code stuff is broken if the test fails.) As I think this PR is a clear improvement, I will merge. Thanks. |
Summary
simplifyapproach compatible (wait for Fix several regressions in 'simplify()' #166)Problem / Motivation
dhnx has support for mixing new and existing pipes in the optimization.
In the current state, dhnx does not return existing pipe segments with the results of the optimization process. This is somewhat logical, if we ask "Dear dhnx, what new pipes do I need to invest in?"
A user can work out their own solution for combining the existing pipes from their input data with the new pipes from the dhnx results. However, to me, this feels very unintuitive. My question would be "Dear dhnx, what does the network look like that supplies heat to all the consumers?"
Furthermore, some information is lost along the way, specifically:
I think dhnx should provide a clean way to get this information, instead of forcing each user to understand and pick apart the oemof results. I found ways to retrieve all those information for the oemof results, but I was unsure how to store them in
network.results.optimization['components']['pipes']My first instinct was to write a solution without breaking changes, where I only add new attributes
status_nominalfrom oemof.This would avoid changing the columns "capacity" and "direction" which can be interpreted as "what is the newly installed pipe capacity"
https://dhnx.readthedocs.io/en/stable/optimization_models.html#results
This prevents breaking changes, but feels less clean. What I now did instead:
Solution
This should avoid breaking changes as far as I can see.
Other noteworthy changes
nominal_valuewithnominal_capacityfillna({"existing": 0}if that column is used in the input data. I ran into issues when I only defined existing=1 for those pipes and left the new pipes NaN.if q["existing"] in [1]:instead ofif q["existing"]:because it evaluates asTrueforNaN. I guess it was related to the point above.bidirectional_pipesis False, new heatpipes are created in both directions, to allow potential flows in both directions. When trying to use existing pipes, I ran into errors like thisValueError: No value for uninitialized VarData object InvestmentFlowBlock.invest[infrastructure_heat_bus_forks-67,infrastructure_heat_pipe-generic_forks-67-forks-75,0]My network became solvable once I added the same logic for exising pipes.
weld_segments()had to be updated, to be compatible with existing pipes.If there are existing pipes in the input data, we do not want to weld/merge/dissolve pipe segments with different capacities, which is what happened before. There is a new attribute
retain_unique_values=['capacity']that makes sure unique values like 'capacity' are preserved.simplify()FutureWarningfor the switch fromweldingtosimplify