Add nonconvex investment flow #826

sasa821 · 2022-03-01T15:42:30Z

Explanation:

This new class represents a new Flow which combines both Investment and NonConvex classes and provides the possibility to perform the capacity optimization of an asset considering min/max loads (i.e., percentages of the capacity which is being optimized), as well as the status of the operation.

It must be noted since in the NonConvexInvestmentFlow a binary variable (status of the flow at each time) is multiplied by a continuous variable, invest, which is the capacity of the asset being optimized, the problem becomes non-linear. For some solvers such as gurobi, this kind of non-linearity is automatically linearized (probably using the big M method), but in other free solvers such as cbc, this causes an error in the optimization. For this reason, those nonlinearities are addressed by introducing a new variable called new_param and adding three new constraints.

Example:

The following diagrams show a good comparison between the Investment and the NonConvexInvestment flows. These diagrams are duration curves of a diesel genset optimized for a mini-grid with the same input for both optimizations. As seen in the figures, assuming that the diesel genset has a minimum load of 20%, the Investment flow would result in an infeasible operation of this device for more than 40% of its annual operation. However, this would never happen using the NonConvexInvestment flow because it considers the minimum load of the genset during the optimization.

Moreover, the optimal capacity using the NonConvexInvestment flow is significantly different from the one obtained from the Investment flow.

pep8speaks · 2022-03-01T15:42:35Z

Hello @sasa821! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-24 14:12:12 UTC

p-snft · 2022-03-02T14:59:53Z

src/oemof/solph/flows/_non_convex_invest_flow.py

+    * :math: `new_param(i,o,t)` (non-negative real number)
+        new paramater representing the multiplication of
+        `P_{invest}` and status(i,o,t)`used for the constraints
+        on the minimum and maximum flow constraints.


What about P_{status}?

What about P_{status}?

I think P_{status} is NOT required here. It is already used in rule definitions for minimum and maximum investment and also in the objective function.

I wanted to suggest to (re)name the parameter new_param (to) P_{status}. As it is the product of the nominal power and the status, the name would make sense, imho.

p-snft · 2022-03-02T15:14:48Z

Hi @sasa821, thanks for your contribution. As far as I see, the code does what it is supposed to do. To be included in the release, unit tests should be included. (Note that the test fails that checks for the error message when combining NonConvex and Investment, you already added a TODO note for that.)

In my opinion, the complexity (cf. Codacy check) is okay. Also, you introduce some code duplication. Both is supposed to be cleaned up later. (We currently have more duplication then necessary, it should not be hard to address the newly induced duplication when we fix the existing one.)

joroeder · 2022-03-02T16:55:28Z

How about naming it NonlinearInvestmentFlow? As it is already possible to make the InvestmentFlow nonconvex for applying investment thresholds, which is something different than here.

sasa821 · 2022-03-02T18:59:09Z

Hi @sasa821, thanks for your contribution. As far as I see, the code does what it is supposed to do. To be included in the release, unit tests should be included. (Note that the test fails that checks for the error message when combining NonConvex and Investment, you already added a TODO note for that.)

In my opinion, the complexity (cf. Codacy check) is okay. Also, you introduce some code duplication. Both is supposed to be cleaned up later. (We currently have more duplication then necessary, it should not be hard to address the newly induced duplication when we fix the existing one.)

Hi @p-snft, thank you very much for your review. Could you assist me more with the inclusion of unit tests, please?

sasa821 · 2022-03-02T19:08:50Z

How about naming it NonlinearInvestmentFlow? As it is already possible to make the InvestmentFlow nonconvex for applying investment thresholds, which is something different than here.

Thanks, @joroeder, for your comment. I used the name NonConvexInvestment because this new class enables the combination of NonConvex and Investment classes, which was not allowed so far in oemof. I understand your point, but I am not sure about NonlinearInvestmentFlow. There is non-linearity in this class, but it is caused by the multiplication of a binary variable and a continuous one. I believe the name NonlinearInvestmentFlow is not very descriptive.

p-snft · 2022-03-14T12:59:44Z

Hi @p-snft, thank you very much for your review. Could you assist me more with the inclusion of unit tests, please?

I'd be glad to help you. However, I am currently lacking time, so it will take some weeks until I can do so.

p-snft · 2022-05-23T19:19:32Z

@sasa821: I'd have time now. Thus, I read through the full PR as it is right now. As using this module is restricted to special solvers, testing will be difficult. Also, I am considering if we should add a warning to the init function, so that nobody uses the feature with an unsupported solver.

Bachibouzouk · 2022-06-01T08:26:53Z

@sasa821: I'd have time now. Thus, I read through the full PR as it is right now. As using this module is restricted to special solvers, testing will be difficult. Also, I am considering if we should add a warning to the init function, so that nobody uses the feature with an unsupported solver.

@p-snft - I just tested it with cbc and it works, I am not sure I understand your point here?

p-snft · 2022-06-01T08:52:45Z

@sasa821: I'd have time now. Thus, I read through the full PR as it is right now. As using this module is restricted to special solvers, testing will be difficult. Also, I am considering if we should add a warning to the init function, so that nobody uses the feature with an unsupported solver.

@p-snft - I just tested it with cbc and it works, I am not sure I understand your point here?

@sasa821 stated that

For some solvers such as gurobi, this kind of non-linearity is automatically linearized (probably using the big M method), but in other free solvers such as cbc, this causes an error in the optimization.

I just believed that without testing it myself. As you say it works, I will test it.

Bachibouzouk · 2022-06-01T12:13:45Z

@sasa821 stated that

For some solvers such as gurobi, this kind of non-linearity is automatically linearized (probably using the big M method), but in other free solvers such as cbc, this causes an error in the optimization.

I just believed that without testing it myself. As you say it works, I will test it.

@p-snft
Because it does not work with Gurobi is one of the reason for the PR :). I commited to the PR and the place where the code is not a copy-paste of either the NonConvexFlow nor the InvesmentFlow is highlighted in the commit 6106a0c, maybe it eases the review from your side.

I also wanted to ask if it would be ok to rebase the PR onto the latest state of oemof:v0.5 or you prefer to see merge commits in the PR?

sasa821 · 2022-06-01T15:57:33Z

@sasa821: I'd have time now. Thus, I read through the full PR as it is right now. As using this module is restricted to special solvers, testing will be difficult. Also, I am considering if we should add a warning to the init function, so that nobody uses the feature with an unsupported solver.

@p-snft - I just tested it with cbc and it works, I am not sure I understand your point here?

@sasa821 stated that

For some solvers such as gurobi, this kind of non-linearity is automatically linearized (probably using the big M method), but in other free solvers such as cbc, this causes an error in the optimization.

I just believed that without testing it myself. As you say it works, I will test it.

@p-snft: I meant that the linearization of the multiplication of a binary and a continuous variable is automatically done in Gurobi, but in cbc this is not the case. Therefore, I added 3 additional constraints to make it possible for cbc to solve the problem. So, the new class is NOT restricted to a specific solver.

p-snft · 2022-06-03T09:23:18Z

I just opened a PR to your branch (@sasa821 ) with two examples for combinations of Invest/NonConvex. I did this just to have something at hand to test if the class works technically. (It does, thank you!) It would be nice if you could provide an example that reproduces the two plots you show in the first post of this PR.

After that, the next step would be adding the tests. Typically, the examples are a good starting points to create tests. This is why I contributed one for NonConvexInvestFlow.

Bachibouzouk · 2022-06-03T12:29:02Z

After that, the next step would be adding the tests. Typically, the examples are a good starting points to create tests. This is why I contributed one for NonConvexInvestFlow.

Thanks @p-snft - So examples are providable within oemof-solph package? I thought one had to store them in the oemof-examples repository (this is the reason why I told @sasa821 we should not commit his example file). Can one add tests where a simulation is run and we look at its output? We already added/modified a few tests in this PR, I am not sure if there is a "good" number of tests to be added ?

p-snft · 2022-06-03T12:33:16Z

We used to have them separately. But as there are severe drawbacks (i.e. results are not tested automatically), this is about to change with v0.5.

When it comes to tests, of course the examples will be checked. This, however, is rather an integration test than a unit test. Thus, we need to add tests for individual functionality. In particular, there should be a constraint test that checks the generated LP file.

Bachibouzouk · 2022-06-13T06:46:07Z

@p-snft - @sasa821 and I plan to wrap up this PR (the work being done now is on the integration and constraint tests, as well as modifying the documentation to have this feature explined) this coming wednesday would you be able/available to review it then? If not could you suggest another core dev of oemof?

p-snft · 2022-06-13T08:33:38Z

docs/usage.rst

+Solph also allows you to model components with respect to more technical details,
+such as minimum power production. This can be done in two different modes: 
+dispatch optimization with fixed capacities and combined dispatch and investment optimization.


As you now allow combining investments and non-convex operation, the "modes" might be confusing. Maybe, tell that we offer different kinds of Flows with different capabilities to increase the computational performance of the model. It's always

(Actually, they were confusing before. Many people though that you could not have NonConvexFlows in an invest optimisation. However, it always worked as long as you do not want to invest in that particular flow.)

p-snft · 2022-06-13T08:43:02Z

I'd gladly do that. I can also offer to not consider problem with docs for solph as a whole (in the sense of #826 (review)) as blockers. Instead, we will rewrite those later.

PS: To be named when the code is cited in scientific publications, you should add your names to https://github.com/oemof/oemof-solph/blob/dev/CITATION.cff. (It was not mentioned in the contribution guidelines, I just fixed that.)

p-snft · 2022-06-16T11:22:29Z

examples/nonconvex_invest_flow_examples/house_with_nonconvex_investment.py

+
+Installation requirements
+-------------------------
+This example requires the version v0.5.x of oemof. Install by:


Suggested change

This example requires the version v0.5.x of oemof. Install by:

This example requires the version v0.5.x of oemof.solph. Install by:

(I know, my fault.)

Done in 3f55df9

src/oemof/solph/flows/_flow.py

p-snft · 2022-06-16T11:37:26Z

src/oemof/solph/flows/_non_convex_invest_flow.py

+        initial_status=0,
+        positive_gradient=None,
+        negative_gradient=None,
+        **kwargs,


Just a note, as your PR aligns with the old/current practice: We want to get rid of "**kwargs". This i.e. will catch typos. The v0.5 way to go is to explicitly define all keyword arguments.

However, you should not expose the "Investment" option as you already did the step to hide the "NonConvex" option. Instead, allow an (explicit) option ep_costs. (It's clear that there is an investment from the choice of the class.)

As NonConvexInvestmentFlow inherits from Flow and Flow itself uses **kwargs, we then might as well define explicitely the argument in Flow, no? Couldn't this be tackled in a separate PR?

I understand the benefit of explicit by getting rid of **kwargs, however wouldn't it make maintenance of inherited classes more cumbersome ? Moreover if typos are caught, then errors in the default values of the arguments in a child class are not caught (a solution for that would be to only pass the argument to the parent class if they don't have the default value of the child class, that way only the default value of the parent class rules.)

You are right, this can be done separately, as the Flow is not updated yet.

(Maybe, this PR is not the right place to discuss about the pros and cons of **kwags. We came to the conclusion to user friendlyness is the stronger argument in the current state of the package. So, all expected arguments are now made explicit. If you want additional arguments, you can still use them, but you have to hand them as a dict to the argument "options".)

src/oemof/solph/flows/_non_convex_invest_flow.py

tests/constraint_tests.py

tests/lp_files/flow_nonconvex_invest_bounded_transformer.lp

tests/test_scripts/test_solph/test_nonconvex_investment/test_nonconvex_investment.py

p-snft · 2022-06-16T14:33:46Z

src/oemof/solph/_exceptions.py

+class FlowOptionWarning(UserWarning):
+    pass
+
+
+class WrongOptionCombinationError(ValueError):
+    pass


We have the other warnings in the tools package, to allow reusing them. It might make sense to do this for these exceptions as well.

(Just as a point for discussion, not a reason to reject this PR.)

I only saw one other warning SuspiciousUsageWarning in oemof.tools. We could define them there if this is needed. I personally find a bit cumbersome to have to create 2 PRs in 2 different packages, but I will abide :)

Maybe, we should also get rid of the SuspiciousUsageWarning in tools... I just brought this up so that the point can be considered in discussions about the overall structure of the oemof packages. (We have a lot of open discussion.)

As this would then be a minor change to move them to oemof.tools or to move SuspiciousUsageWarning in _exceptions.py, could we leave it as is for now and act when the open discussion comes to a conclusion :) ?

Describe the change introduced by the NonConvexInvestFlow Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Since the new parameter `invest_non_convex` exists in the `NonConvexInvestFlow`, there is no need for the `invest_status`. Moreover, usage of the `NonConvexInvestFlow` class indicates that the `invest` always exists. Therefore, `invest_status` can be removed and by removing it, the computation time would increase. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

The sets `NON_FIXED_INVESTFLOWS` and `FIXED_INVESTFLOWS` and their corresponding constraints are removed because they seem to be unnecessary in the `NonConvexInvestFlow` and removing them would increase the computation time. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Change the `min`, `max`, and `conversion_factors` values to numbers that can be cast from decimal to floating-point and back (e.g., 0.25, 0.5, ...) to increase the readability of the corresponding *.lp file. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Explicitly name authors in the "SPDX-FileCopyrightText". Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

In some parts, the name of the `NonConvexInvestFlow` was written incorrectly. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Modify the example file because it is not read from somewhere else. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Bachibouzouk · 2022-06-22T10:15:46Z

@p-snft - we implemented the changes you suggested, with exception of the parts we agreed could be tackled in subsequent PRs.
You appear explicitly as coauthor of one commit, do you have a noreply github address that you would prefer to use there?

There is also need to approve running the workflows of the PR

Bachibouzouk · 2022-06-24T10:27:35Z

@uvchik - I just saw the PR close automatically because the oemof:v0.5 branch was deleted, is it then moved to another branch ? What should we then do with the non_convex branch which we wanted to merge into oemof-solph?

From #854 I understand now that dev replaces the oemof:v0.5 branch

Bachibouzouk · 2022-06-24T12:23:27Z

@p-snft , @uvchik - could you reopen this PR and set the target branch to dev? I don't have the rights to do so

p-snft · 2022-06-24T14:17:20Z

I hat to reopened the v0.5 branch to be able to reopen this PR. Afterwards, I could rebase it. Done.

PS: I do not use a github address for my contributions. I'm fine with the address in the commit log.

p-snft

In my opinion, the remaining issues have to be solved rather in the original solph Flows then in this PR.

Documentation of the Flow classes: I.e. the non-convex "mode" and investment "mode" don't really exist since there is NonconvexFlow and InvestmentFlow.
Code duplication: The flow classes should be more modular. But this is nothing to be fixed here.
Test coverage: There are missing tests for duplicated code. This will solve as soon as the duplication is removed by making the original flow more modular.

So, I'd agree to merge this. (If a second reviewer has a problem with temporarily decreasing test coverage, moving the NonConvexInvestmentFlow into a subdirectory called "experimental" might help. We do the same thing for components.)

Bachibouzouk · 2022-06-24T16:24:59Z

Thanks @p-snft, now the target branch is correct, should someone else review this PR? Or merge it?

p-snft · 2022-06-24T20:34:45Z

I asked for a second reveiw in the dev chat. For non-trivial stuff we want to have more then one review if possible.

docs/usage.rst

joroeder

First of all, I think that is really nice contribution. Thanks to all people that were involved.

I would appreciate a feedback on the comments in the files.

General remarks (that should not hinder the merge):

I am not that deep into grouping, so I cannot say if everything is fine there.
Documentation should undergo an overall revision anyway, but this is a separate issue -- so, very nice contribution!
Local build of the API documentation did not work - is it a problem of this PR or with the dev branch? (or at my machine?)
Do we need the init.py in the examples? The others do not have an init either, so remove it?
The repository will become very large if we include many examples with a dataset of 8760 time steps -- Is this wanted?

joroeder · 2022-06-27T13:34:14Z

tests/test_scripts/test_solph/test_nonconvex_investment/diesel_genset_data.csv

@@ -0,0 +1,8761 @@
+Demand,SolarGen


I think it is not a good practice to have such large files for testing purposes. Do you really need that? Could you reduce this to 10 time steps?

I agree that we need to trim it to the minimum time required to be able to see the difference with the non_convex without invest problem

joroeder · 2022-06-27T13:50:14Z

src/oemof/solph/flows/_non_convex_invest_flow.py

+            investment_costs += (
+                self.invest[i, o] * m.flows[i, o].investment.ep_costs
+                + m.flows[i, o].investment.offset
+            )


Suggested change

investment_costs += (

self.invest[i, o] * m.flows[i, o].investment.ep_costs

+ m.flows[i, o].investment.offset

)

investment_costs += (

self.invest[i, o] * m.flows[i, o].investment.ep_costs

)

The offset simply adds a fixed value to your objective independent of the investment decision. I think this should be removed.

src/oemof/solph/flows/_non_convex_invest_flow.py

Bachibouzouk · 2022-06-27T21:03:54Z

Do we need the init.py in the examples? The others do not have an init either, so remove it?

This was probably left from a failed attempt to reuse the example in the tests (so one would need to be able to import the example)

p-snft reviewed Mar 2, 2022

View reviewed changes

p-snft added this to the v0.5.0 milestone Mar 14, 2022

uvchik assigned p-snft May 23, 2022

sasa821 closed this Jun 1, 2022

sasa821 reopened this Jun 1, 2022

Bachibouzouk force-pushed the dev_non_convex_invest_flow branch from 7981b92 to e2e11af Compare June 1, 2022 16:23

p-snft reviewed Jun 13, 2022

View reviewed changes

Bachibouzouk force-pushed the dev_non_convex_invest_flow branch 2 times, most recently from f7b67d9 to daebfa4 Compare June 16, 2022 10:39

p-snft reviewed Jun 16, 2022

View reviewed changes

Bachibouzouk and others added 4 commits June 22, 2022 11:53

Update documentation

40666b8

Describe the change introduced by the NonConvexInvestFlow Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Add names to authors

10e5786

Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Add new exception/warning

ae2f5d8

Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Add tests to defined expectation of NonConvexInvestFlow

e125d3a

Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

sasa821 and others added 8 commits June 22, 2022 11:55

Name authors

927eb02

Explicitly name authors in the "SPDX-FileCopyrightText". Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Correct the name of the new class

d2ebb87

In some parts, the name of the `NonConvexInvestFlow` was written incorrectly. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Update the example file

6093465

Modify the example file because it is not read from somewhere else. Co-Authored-By: Pierre Francois <4399407+Bachibouzouk@users.noreply.github.com>

Rename oemof.solph

3f55df9

Make isort happy

cc747bd

Co-authored-by: sasa821 <83787088+sasa821@users.noreply.github.com>

Bachibouzouk force-pushed the dev_non_convex_invest_flow branch from 2dec8cf to cc747bd Compare June 22, 2022 10:04

uvchik deleted the branch oemof:feature/restucture_flow June 24, 2022 10:01

uvchik closed this Jun 24, 2022

p-snft reopened this Jun 24, 2022

p-snft changed the base branch from v0.5 to dev June 24, 2022 14:26

p-snft approved these changes Jun 24, 2022

View reviewed changes

p-snft requested a review from a team June 24, 2022 20:33

joroeder reviewed Jun 27, 2022

View reviewed changes

docs/usage.rst Show resolved Hide resolved

joroeder requested changes Jun 27, 2022

View reviewed changes

p-snft changed the base branch from dev to feature/restucture_flow June 29, 2022 12:25

p-snft merged commit 43cfc14 into oemof:feature/restucture_flow Jun 29, 2022

sasa821 mentioned this pull request Oct 14, 2022

Feature/restucture flow -- upgrade OffsetTransformer #868

Closed

p-snft mentioned this pull request Oct 19, 2022

2022 (November) dev meeting oemof/oemof#100

Closed

sasa821 mentioned this pull request Nov 23, 2022

Upgrade OffsetConverter #895

Merged

eikohi mentioned this pull request Mar 17, 2023

Combination of Non-Convex and Investment optimization in the same flow SESMG/SESMG#168

Open

	This example requires the version v0.5.x of oemof. Install by:
	This example requires the version v0.5.x of oemof.solph. Install by:

Add nonconvex investment flow #826

Add nonconvex investment flow #826

Conversation

sasa821 commented Mar 1, 2022

Explanation:

Example:

pep8speaks commented Mar 1, 2022 • edited Loading

Comment last updated at 2022-06-24 14:12:12 UTC

Choose a reason for hiding this comment

sasa821 Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-snft commented Mar 2, 2022

joroeder commented Mar 2, 2022

sasa821 commented Mar 2, 2022

sasa821 commented Mar 2, 2022

p-snft commented Mar 14, 2022

p-snft commented May 23, 2022

Bachibouzouk commented Jun 1, 2022

p-snft commented Jun 1, 2022

Bachibouzouk commented Jun 1, 2022 • edited Loading

sasa821 commented Jun 1, 2022

p-snft commented Jun 3, 2022

Bachibouzouk commented Jun 3, 2022

p-snft commented Jun 3, 2022

Bachibouzouk commented Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

p-snft commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bachibouzouk commented Jun 22, 2022 • edited Loading

Bachibouzouk commented Jun 24, 2022 • edited Loading

Bachibouzouk commented Jun 24, 2022

p-snft commented Jun 24, 2022 • edited Loading

p-snft left a comment

Choose a reason for hiding this comment

Bachibouzouk commented Jun 24, 2022

p-snft commented Jun 24, 2022

joroeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bachibouzouk commented Jun 27, 2022

pep8speaks commented Mar 1, 2022 •

edited

Loading

sasa821 Mar 2, 2022 •

edited

Loading

Bachibouzouk commented Jun 1, 2022 •

edited

Loading

Bachibouzouk commented Jun 13, 2022 •

edited

Loading

Bachibouzouk commented Jun 22, 2022 •

edited

Loading

Bachibouzouk commented Jun 24, 2022 •

edited

Loading

p-snft commented Jun 24, 2022 •

edited

Loading