Feature/restucture flow

[p-snft]

Notes: We need to either include #799 or this makes it obsolete.

[Bachibouzouk]

Discussion with @p-snft , @joroeder , @sasa821 : there should not be InvestFlow, NonConvexFlow, NonConvexInvestFlow but just one Flow class and depending on the argument the user provides, the Flow can be nonconvex and/or with invest mode.

@Bachibouzouk maybe the design pattern we want to apply here is similar to this one: https://refactoring.guru/design-patterns/strategy

We should write pseudo code call with different usecase to be able to decide what should happen in each case and implement tests before refactoring

[p-snft]

Because NonConvex and Investment are no longer mutually exclusive, we may rethink the structure of Flow, NonConvexFlow and InvestmentFlow, because there is NonConvexInvestmentFlow. For the API, it might be more intuitive to have Invest and NonConvex as options. (In my memory we wanted to get rid of the options because they were confusing because they are exclusive.)

[Bachibouzouk]

To define a nonconvex and investment flow

solph.flows.Flow(
    nominal_value=None,
    variable_costs=...,
    min=<number in (0,1)>,
    max=<number in [min,1)>,
    investment=solph.Investment(
        ep_costs=...,
        maximum=..., # need to have maximum provided here
    )
)

Another alternative

solph.flows.Flow(
    nominal_value=None,
    variable_costs=...,
    min=<number in (0,1)>,
    max=<number in [min,1)>,
    ep_costs=...,
    investment_maximum=..., # need to have maximum provided here
    )
)

To me it should be possible to not explicitly provide nominal_value

solph.flows.Flow(
    variable_costs=...,
    min=<number in (0,1)>,
    max=<number in [min,1)>,
    ep_costs=...,
    investment_maximum=..., # need to have maximum provided here
    )
)

A special case which falls back on convex investment flow case

solph.flows.Flow(
    variable_costs=...,
    min=0,
    max=1,
    ep_costs=...,
    investment_maximum=...
    )
)

[p-snft]

Generally, I see that all the alternatives are possible. (There are more, but more on that later.) Especially, I agree that it should be possible to not explicitly provide nominal_value. Also, if a nominal_value and an invest, that can be interpreted as existing capacity that might be increased.

I want to discuss some more open questions (Qn) using the following example:

solph.flows.Flow(
    nominal_value=<number>,
    variable_costs=...,
    min=[0 <= float <= max],  # Should a minimum imply NonConvex? (Q1)
                              # Until now, setting a min without NonConvex means always on.
    max=[min <= float],  # Currently, max can be equal to min and can also exceed 1.
                         # Should it stay like that? (Q2)
    fix=[min <= float <= max],  # Should it be possible to have a series like [0, 0, .5, None, .2],
                                # where there is not fixed value for step 4? (Q3)
                                # If yes: Should it be possible to define min, max, and fix at the same time? (Q4)
)

As I like the current solution for Q1 (minimum also excludes value of 0, always on when no NonConvex is set), I'd suggest to stick with that. However, the argument might be called e.g. binary_status, to be easier to understand for non-mathematicians.

PS: The arguments positive_gradient and negative_gradient are present in both, Flow and NonConvex. I have the feeling that they are only required once.

[jokochems]

Thanks for opening this discussion.

In general, I like the idea of having one single Flow class.
I don't like putting arguments into a flat argument structure. I think, it is much more intuitive and less error prone to use a nested structure, i.e. if you want an investment to happen, in my view, the most intuitive form is something like

solph.flows.Flow(
    nominal_value=None,
    variable_costs=...,
    min=<number in (0,1)>,
    max=<number in [min,1)>,
    investment=solph.Investment(
        ep_costs=...,
        maximum=..., # need to have maximum provided here
    )
)

Also, I'd strongly suggest to split after v0.5. I seriously see #810 at risk if we keep integrating new structural changes.

[jokochems]

I'm not going to go into details with a code review and at the moment, unfortunately, my time for oemof development is very limited, but I had a quick look and really like the structural improvements, this PR brings along. I'm a bit worried about integrating #828, though, but I'll try to cut some time in October to resolve merge conflicts and copy & paste the changes brought along. ;-)

[p-snft]

Were examples/nonconvex_invest_flow_examples/

• diesel_genset_nonconvex_investment.py
• house_with_nonconvex_investment.py
• house_without_nonconvex_investment.py
  ran?

I would search for occurences of NonConvexInvestFlow and replace them by NonConvexInvestmentFlow

I see that the class InvestNonConvexFlowBlock is often referred to as NonConvexInvestFlowBlock in docstrings and comments

Thanks for your feedback. Adding to these points, I also forgot to align the naming if the example with the new wording.

Done. Remaining tasks are addressed in #863.

[joroeder]

When going through the changes, I updated the links in docs. (which is also part of #863, and made some minor suggestion for improvement.

Do we need an __init__.py in the examples. I think this is not necessary and we should remove the file.

Besides, I do not see anything against merging this PR.

[joroeder]

The following paragraphs need to be revised as it describes an outdated state. I have tried to address this, and to link the right API documentation of the blocks (a short check would be nice). I think we at least should not merge code snippets with a wrong API.

Generally, I think it is really nice that there is something about the mathematical background.

[p-snft]

As far as I see, there is no objection and one positive review.As some PRs already build on the changes presented here, I will now merge. Remaining clean-up (of the documentation and adjustments to the other flow classes) is addressed in #863.