New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple the investment of storage flows and capacity #480

Merged
merged 8 commits into from May 16, 2018

Conversation

Projects
None yet
3 participants
@FabianTU
Copy link
Contributor

FabianTU commented May 1, 2018

The old GenericInvestmentStorageBlock had a fixed Capacity Ratio that had to be initialized and thus you could not solely let oemof invest into the Input/Output Flow.

By adding the if-Statements, it is now possible to have a storage flow that does not influence the capacity of the storage.

@uvchik uvchik self-requested a review May 2, 2018

@uvchik uvchik self-assigned this May 2, 2018

@uvchik uvchik added this to the v0.2.2 milestone May 2, 2018

@uvchik uvchik added the enhancement label May 2, 2018

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 2, 2018

I think the main target of this PR is to decouple the initialisation of the storage and the initialisation of the Flow.

Your code does not work at all caused by wrong indentation. Anyhow I would rather define sets than placing if-statements inside the rules. Have a look at the initial_capacity to see how it works.

        self.INITIAL_CAPACITY = Set(initialize=[
            n for n in group if n.initial_capacity is not None])

[...]

        def _initial_capacity_invest_rule(block, n):
            """Rule definition for constraint to connect initial storage
            capacity with capacity of last timesteps.
            """
            expr = (self.capacity[n, m.TIMESTEPS[-1]] == (n.initial_capacity *
                                                          self.invest[n]))
            return expr
        self.initial_capacity = Constraint(
            self.INITIAL_CAPACITY, rule=_initial_capacity_invest_rule)

In contrast to the _storage_balance_rule the _initial_capacity_invest_rule is not created for the set self.INVESTSTORAGES but only for the subset self.INITIAL_CAPACITY which is defined above with the if-statement. It is easier to understand for others because you will have an overview over all sets at one place.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 2, 2018

Hey, I think decoupling power / energy investment ist great. But how would the API look like for energy (or capacity) part of the investment costs and the power part of the investment costs?

storage = solph.components.GenericStorage(
    label='storage',
    inputs={bel: solph.Flow(solph.Investment(ep_costs=epc_storage_power_in),
                                          variable_costs=0.0001)},
    outputs={bel: solph.Flow(solph.Investment(ep_costs=epc_storage_power_out)},
    capacity_loss=0.00, initial_capacity=0,
    nominal_input_capacity_ratio=None,
    nominal_output_capacity_ratio=None,
    inflow_conversion_factor=1, outflow_conversion_factor=0.8,
    investment=solph.Investment(ep_costs=epc_storage_capacity),
)

We have to decide how to deal with input / output power...in the example above, the storage could be setup with different capacity and input output power based on specific costs for each of these... But I think the 'normal' way would be to have capacity related costs and power related costs (only once), where input / output power are the same (like if you would have only on bidirectional flow with a maximal value)...

Hope you understand the problem i am talking about..

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 2, 2018

@simnh I think your example is exactly how it is meant to be.

I do not know if I got you right but it comes from a PHES where you can define costs for the pump, the turbine and for extending the basin (the discussion started in the forum).

@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 3, 2018

@simnh that is exactly the way I intended it to work. Why exactly shouldn't it possible to invest into Input and Output flow autonomously? To cover also that case, you theretically could fix the input to the output investment and then assume half the costs for each. But that again would require another set I think. But I see your point..

@uvchik , the indentation mistake may happened during the PR. I changed the Code online in the Editor. For me the Code works and does what it is supposed to be. Anyhow, I will try to change the approach according to the example "initial_capacity".

Maybe as an indicator for the complexity: I currently have 6 kind of Storages in my system, 4 of them had the issue that I wanted to decouple the investment in capacity and power. Thus, I had a Transformer for Input and a Transformer for the Output plus a "dummy"-bus to connect all of them ( as can be seen in the thread Uwe linked).
Before optimization: 430seconds
After optimization: 317 seconds
Solver: cbc

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 8, 2018

Before optimization: 430seconds
After optimization: 317 seconds
Solver: cbc

Does this mean, the new storage is faster than your solution with the transformers?

What is missing for this feature, when will it be available...very interested ;-)

@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 8, 2018

Yes it seems to be way faster, within my example at least :) For now the changes are quite minor, but do save many constraints that you would need otherwise (I think - I am by no means an expert here).

I uploaded the new version 2 days ago and its currently awaiting a review. Not sure how the process works entirely..

But as you mentioned I think there is one option missing, fixing the Input Power to the Output Power (for example a Redox-Flow Battery). I will think about that tomorrow in more detail.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 8, 2018

Ok great!

And of course the current functionality should work (i.e. capacity costs only and a ratio for capacity / power )

Regarding the process:

  • Make sure all current test work (you can test this by running 'nosetests --with-doc on your machine
  • Maybe add another test (but this can also be done by some else
  • Add documentation
  • Add all the information to the 'whats-new' file in the /doc directory
@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 9, 2018

One question to get your opinion:

I now want to implement the possibility that oemof can freely invest in capacity and power BUT Input and Output power has to be the same value.
For that I created a new variable in the GenericStorage-constructor (power_coupled ; 1 or 0 to choose). From that I generate a constraint. So far so good.
I was wondering if it would proove more practically and more easy to understand for a user to extend/Change the variable to a more generic Approach
e.g. invest_type --> "power_coupled" ; "capacity_power_coupled" and "nothing_coupled". Those key words then would define whether or not variables such as capacity_input_ratio is used or not.

For now you have to set the capacity Ratio to "None" for oemof to know that power is decoupled from capacity.

I hope you understand what I am thinking about - basically just in which way the storage is easier to understand and used correctly.

For the process: Could you specify on how to run the test? If I simply copy and paste the Code ('nosetests --with-doc), I retrieve an error.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 9, 2018

I think key words in the storage class would be a very good idea! (though I would prefer something else than 'nothing_coupled'), for example:

For the API something like: couple_investment and possible values to set: power, power_capacity, None

Where as None is the default which we currently have...

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 9, 2018

One Idea: I would also like to have the option of 'cyclic' for setting the storage level at the start and end to the same value or, if False, to leave it open (current implementation is always cyclic). Do you think we could integrate this here? or at least kind of together with this one?

@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 10, 2018

I changed the wording to your proposals, I think they fit very good. I also added two Errors if the Input seems to be contradictory.

I am not sure if I understand your idea. Do you mean a certain percentage of the Maximum capacity? For now you have the possibility to use the initial_capacity Parameter which Forces oemof to end and start with e.g. the value 0.

@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 12, 2018

I changed the set 'initial_capacity' which is responsible for the constraint that Forces the cyclic behaviour. Now it should be possible that the storage can still have e.g. electricity stored.

Out of curiosity, do you have a rolling horizon model in mind or why would you need that option? Atleast, that is what I have in mind and what I will use it for..

Can anybody tell me why exactly the TravisCI check failed? I can't find the mistake and the file does work for me without any issues.

FabianTU added some commits May 13, 2018

@uvchik uvchik changed the title Update components.py Decouple the investment of storage flows and capacity May 14, 2018

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 14, 2018

Changing the connection of the capacity at the first and last time step should be another PR. Therefore, I changed the title to make it clearer. Otherwise the whole PR is becoming confusing.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 14, 2018

We really have to design a new API instead of quick additions and removals.

At the moment we have to way to define the capacity of the in/out flows. We can define a nominal value in the flows or the nominal_input_capacity_ratio (or output). I think it would avoid errors/problems if we just allow one way. Assuming we would decide that the nominal_value is the default way we could remove the attribute nominal_input_capacity_ratio (and output). We still need values to connect the flows in the invest mode. Therefore I would add three new attributes to connect input-capacity, outptut-capacity and input-output in the invest mode.

In summary the new API for v0.3 could look like this (just my proposal). We need to keep the API for v0.2.x but this should not be a problem.

     Parameters
     ----------
     nominal_capacity : numeric
         Absolute nominal capacity of the storage
     initial_capacity : numeric or None
         The capacity of the storage in the first (and last) time step of
         optimization.
     capacity_loss : numeric (sequence or scalar)
         The relative loss of the storage capacity from between two consecutive
         timesteps.
     inflow_conversion_factor : numeric (sequence or scalar)
         The relative conversion factor, i.e. efficiency associated with the
         inflow of the storage.
     outflow_conversion_factor : numeric (sequence or scalar)
         see: inflow_conversion_factor
     capacity_min : numeric (sequence or scalar)
         The nominal minimum capacity of the storage as fraction of the
         nominal capacity (between 0 and 1, default: 0).
         To set different values in every time step use a sequence.
     capacity_max : numeric (sequence or scalar)
         see: capacity_min
     investment : :class:`oemof.solph.options.Investment` object
         Object indicating if a nominal_value of the flow is determined by
         the optimization problem. Note: This will refer all attributes to an
         investment variable instead of to the nominal_capacity. The
         nominal_capacity should not be set (or set to None) if an investment
         object is used.
-    nominal_output_capacity_ratio : numeric
-        Ratio between the nominal outflow of the storage and its capacity. For
-        batteries this is also know as c-rate.
-        Note: This ratio is used to create the Flow object for the outflow
-        and set its nominal value of the storage in the constructor. If no
-        investment object is defined it is also possible to set the nominal
-        value of the flow directly in its constructor.
-    nominal_input_capacity_ratio : numeric
-        Ratio between the nominal inflow of the storage and its capacity.
-        see: nominal_output_capacity_ratio
+   invest_relation_input_output: numeric or None
+       Relation of the investment variables between input and output.
+       input_invest = output_invest * invest_relation_input_output
+   invest_relation_input_capacity: numeric or None
+       Relation of the investment variables between input and capacity.
+       input_invest = capacity_invest * invest_relation_input_capacity
+   invest_relation_output_capacity: numeric or None
+       Relation of the investment variables between output and capacity.
+       output_invest = capacity_invest * invest_relation_output_capacity

If the invest relation is None to connection will be created. Now there is only one way to define the storage for both ways and no contradictory attributes.

@simnh @FabianTU What do you think?

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 14, 2018

@FabianTU Please have a look at our developing section in the documentation.

According to the documentation you have to execute nosetests -w "/path/to/oemof" --with-doctest to run the tests. Run the test in the same python environment in which you use oemof, otherwise there might be errors due to missing packages.

To learn a bit about commit messages please have a look here.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 14, 2018

I agree that we need a new API instead of fixing, but I like the idea from above more than implicit setting / not setting of constraints etc. by None or not None. Therefore I think the investment_coupled attribute is kind of the right direction. I would also be fine with a complete new API, as the wording is quite clumsy anyway ...

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 14, 2018

Changing the connection of the capacity at the first and last time step should be another PR. Therefore, I changed the title to make it clearer. Otherwise the whole PR is becoming confusing.

Yes that's what I meant, so I would add a new pull request, but it will be connected to the new API so maybe we can do it here?

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 14, 2018

I agree that we need a new API instead of fixing, but I like the idea from above more than implicit setting / not setting of constraints etc. by None or not None. Therefore I think the investment_coupled attribute is kind of the right direction. I would also be fine with a complete new API, as the wording is quite clumsy anyway ...

For me it is in no implicit at all. If I set the connection between two variables to None for me it is very explicit that this connection is None (not existing). For me is is more confusing to set a value for the connection between two variables and then have to enable this connection with a second attribute.

My use cases are the following (PHES):

  1. Nothing is connected
storage = solph.components.GenericStorage(
    [...]
    inputs={bel: solph.Flow(solph.Investment(ep_costs=epc_storage_power_in))},
    outputs={bel: solph.Flow(solph.Investment(ep_costs=epc_storage_power_out))},
    invest_relation_input_output=None,
    invest_relation_output_capacity=None,
    invest_relation_input_capacity=None,
    investment=solph.Investment(ep_costs=epc_storage_capacity),
)
  1. Turbine and Pump will always have the same size
[...]
    invest_relation_input_output=1,
    invest_relation_output_capacity=None,
    invest_relation_input_capacity=None,
)
  1. Scale the whole storage and keep the ratio.
[...]
    invest_relation_input_output=None,
    invest_relation_output_capacity=0.10,
    invest_relation_input_capacity=0.13,
)

... and so on. For me this looks clear.

The problem is that it is still be possible to over-define the Object. Therefore, we could raise an error if all three attributes are not None.

[...]
    invest_relation_input_output=1,
    invest_relation_output_capacity=0.10,
    invest_relation_input_capacity=0.13,
)

The code above works but it is confusing as one relation will be useless.

@simnh I think the second use case is not possible with your API. Could you provide examples?

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 14, 2018

Yes that's what I meant, so I would add a new pull request, but it will be connected to the new API so maybe we can do it here?

@simnh You could open an issue, where we discuss the API. We could implement it once this PR is working to avoid merge conflicts.

@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 14, 2018

I think completely new atributes is the best way to avoid confusion and I thought of doing that as well. My Intention was to enable the use of older versions and to Change as Little as possible but enable all possibilities.

To your use cases: The second Option would be enabled by Setting the Attribute couple_investment='power' and also nominal_input/Output_capacity_ratio=None (I added an error if the nominal_input_capacity_ratio is not None as well).

I think both approaches have their upsides, but for me I don't like to initialize an Attribute with a string. Also, having an integer/double/float it enables to also adjust the size of both with that very value (as you indicated in the documentation) and gives more flexibility and possibilities.

To clarify your comments (because the last one confused me a Little): Shall I Change it according to your suggestions but exclude the cycling Option to avoid confusion with the PR?

Thanks for your effort btw - fun to work on this even though it seems very clumsy on my part ;)

@simnh simnh referenced this pull request May 14, 2018

Merged

Features/solph existing investment #489

5 of 5 tasks complete
@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 15, 2018

@FabianTU: for me the current implementation is ok! As I said, I would like to redesign the storage API anyway in the future. For now we have nice new functionality with your proposal!

Regarding cylic: (I think) you can leave this here as well I think, as it is just another option. However, it would be good, if this would also be added to the GenericStorage as well.

The docstrings of the 'blocks' would need an update with the additional constraints, sets etc.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 15, 2018

@uvchik Could we integrate this branch into the oemof repo, so that we or I can also push some stuff. We could then help to finalize this PR...

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 15, 2018

With the new API it is possible to decide which parts of the storage are fixed and in which parts can be extended with the investment mode (e.g. pump, turbine, basin for a PHES). I really like this.

Let us assume that the basin is fixed and the turbine and pump can be extended. The turbine should always be 50% bigger than the pump due to whatever reason.

 storage = solph.components.GenericStorage(
    [...]
    inputs={bel: solph.Flow(solph.Investment(ep_costs=epc_storage_power_in))},
    outputs={bel: solph.Flow(solph.Investment(ep_costs=epc_storage_power_out))},
    invest_relation_input_output=0.6667,
    invest_relation_output_capacity=None,
    invest_relation_input_capacity=None,
    nominal_capacity=5000,
)

@FabianTU Could you please present an example, how it would look in your API.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 15, 2018

I would like to redesign the storage API anyway in the future.

What does that mean?

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 15, 2018

Thanks for your effort btw - fun to work on this even though it seems very clumsy on my part ;)

This is how it works and hopefully we have a nice API after the discussion.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 15, 2018

Regarding cylic: (I think) you can leave this here as well I think, as it is just another option. However, it would be good, if this would also be added to the GenericStorage as well.

I disagree because the discussion is already becoming confusing. Why not opening an extra PR for an extra feature.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented May 15, 2018

I disagree because the discussion is already becoming confusing. Why not opening an extra PR for an extra feature.

Because: a) it's not a feature for me but only a small enhancement and b) it's more time consuming

I propose: leave it here unless someone opens a new PR with the code and all information necessary (I do not have time for this right now)

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 15, 2018

Because: a) it's not a feature for me but only a small enhancement and b) it's more time consuming

I propose: leave it here unless someone opens a new PR with the code and all information necessary (I do not have time for this right now)

Then I would just add a boolean attribute: cycle. True: First time step is equal to last time step, False: First time step does not have to be equal to last time step (no constraint).

If initial_capacity is None the first time step is free otherwise it is bound to initial_capacity.

@FabianTU FabianTU closed this May 15, 2018

@FabianTU FabianTU reopened this May 15, 2018

@FabianTU

This comment has been minimized.

Copy link
Contributor Author

FabianTU commented May 16, 2018

Changed the API to @uvchik proposals. Documentation included and nosetests worked as well.
It should now be possible to:

  • Decide whether or not cycling is True/False

  • How Input/Output power and capacity is connected. It is also possible to couple Input Power to the capacity and to also couple Output power to the Input power.

PHES_Speicher = solph.components.GenericStorage(
        inputs = {bel: solph.Flow(investment=solph.Investment(ep_costs=epc_phes_leistung_pumpe), variable_costs=1)},
        outputs = {bel : solph.Flow(investment=solph.Investment(ep_costs=epc_phes_leistung_pumpe), variable_costs=1)},
        label="PHES_Speicher",      
        inflow_conversion_ratio=1,      
        outflow_conversion_ratio=1,      
        nominal_input_capacity_ratio=0.5,     
        nominal_output_capacity_ratio=None,   
        capacity_loss=0,    
        initial_capacity=0,   
        investment=solph.Investment(ep_costs=epc_phes_kapazitaet),
        invest_relation_input_output_power = 5/6,     
        cyclic=False
        )

The Change does not have any influence on the older versions (Hopefully). Error will raise if all attributes are set: nominal_input_capacity_ratio, nominal_out_capacity_ratio and invest_relation_input_output_power

@uvchik uvchik changed the base branch from dev to features/decouple_flows_capacity_investment_storage May 16, 2018

FabianTU and others added some commits May 16, 2018

@uvchik uvchik merged commit f829be3 into oemof:features/decouple_flows_capacity_investment_storage May 16, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 17, 2018

@FabianTU Thank you for your contribution I will add some test and merge it into dev soon (see #491).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment