oemof / oemof-solph

A model generator for energy system modelling and optimisation (LP/MILP).
https://oemof.org
MIT License
294 stars 125 forks source link

Word "Capacity" in GenericStorage can be misleading #538

Closed p-snft closed 4 years ago

p-snft commented 5 years ago

Current renaming due to this ticket:

capacity_loss -> loss_rate
initial_capacity -> initial_storage_level (between 0 and 1)
capacity_max -> max_storage_level (between 0 and 1)
capacity_min -> min_storage_level (between 0 and 1)
nominal_capacity -> nominal_storage_capacity (an Energy)

Original comment: In the context of the GenericStorage, Capacity can refer to different things. From my perspective, the word is used inconsistently:

The problem is known, and somewhat addressed in the documentation: "Investment in this context refers to the value of the variable for the 'nominal_value' (installed capacity) in the investment mode."

Capacity is often used synonym to storage capacity. This holds for batteries [J], and even water basins (in litres, when not used as pumped hydroelectric energy storage). For thermal energy storage, I found capacity used the same way. (Note also heat capacity is a fixed term defined in units of J/K. For thermal reservoirs the temperature is typically assumed to be constant.) Concluding, I would opt to rename capacity_loss and initial_capacity.

Suggestions:

simnh commented 5 years ago

I like your suggestions!!

One additional thought: We actually decided to use capacity for power and storage_capacity for energy in our models. Typically capacity is often used in energy system analysis context for power related values like the capacity of the powerplant. We found it very useful to use the naming as described. This makes also investment more precise in terms of naming, as capacity investment is then always for power related values...

Therefore maybe nominal_capacity might also be changed to nominal_storage_capacity

But I do not want to make the discussion to complicated. As I said: I vote for the suggestions above!

p-snft commented 5 years ago

Thanks for your feedback. I also support renaming nominal_capacity to nominal_storage_capacity, as this would make things even clearer.

uvchik commented 5 years ago

We are revising the code of the GenericStorage class in PR #493. I would integrate the result of this decision there to avoid unnecessary code conflicts.

I am fine the new names.

ckaldemeyer commented 5 years ago

I am fine the new names.

Me too!

lmilletb commented 5 years ago

If I may add my opinion, I also think this improves readibility and comprehensibilty of the Storage blocks!! However, I thought it would be even more clear if it would be named "self discharge rate" (or simply "self discharge") instead of "loss rate". Aside, a decreasing "nominal_capacity" from a storage block is something common due to the degradation of the storage (battery?)... I believed for many weeks that "capacity loss" referred to that, instead of the self-discharge. Now it is already clear but... I think a "capacity fade rate" would still be very useful for the users. The implementation should be simple as the "nominal_capacity" sequence could be generated taking into account the "capacity fade rate". What is your opinion? If you agree, I could try to implement and commit necessary changes to PR #493.

uvchik commented 5 years ago

We have the capacity_max attribute, which can be a time series. This makes it possible to use any kind of capacity fade function. With a fade rate it will just be linear.

uvchik commented 5 years ago

But of course we could add this information to the documentation because people may not understand that they can use this attribute in that way.

p-snft commented 5 years ago

In total, we now have discussed to rename the following parameters:

To retain consistency with the initial_storage_level, also capacity might be renamed.

**The following variables are created:**
capacity
    Capacity (level) of the storage and time step. The capacity is bound to

    .. math::
        nominal\_capacity \cdot capacity\_min(t) < capacity(t) <
        nominal\_capacity \cdot capacity\_min(t)

Still, initial_capacity might remain. This seems also OK to me, since renaming nominal_capacity to nominal_storage_capacity already addresses one source of possible confusion. (Also: storage_level might be misunderstood as a value between 0 and 1.)

PS: I found that the initial_capacity/initial_storage_level in fact is not an energy, but the derived (internal) variable initial_cap is one.

p-snft commented 5 years ago

During implementation I found some other sources of confusion. (Actually, I was confused in the post above.) To improve consistency and readability, I would suggest that

For example, compare

nominal_capacity * capacity_min[t]
    < capacity
    < nominal_capacity * capacity_max[t]

(capacity² < capacity < capacity²) and

nominal_storage_capacity * min_storage_level[t]
    < capacity
    < nominal_storage_capacity * max_storage_level[t]

For the time being, I will start with the renamings we already decided on. They do perfectly fit into the scheme.

p-snft commented 5 years ago

Done. Will be merged together with #493.

jnnr commented 4 years ago

I re-read the above discussion in the context of PR625 and agree with the decisions made above. In my view, the name changes applied in the beginning of the year are very helpful!

For the time being, I will start with the renamings we already decided on. They do perfectly fit into the scheme.

It seems that there are still some open ends. Therefore I reopen the issue, mainly because of one point that has been discussed already, the attribute capacity.

In my opinion it would make sense to rename capacity to energy_content or storage_content (for a generic storage, e.g. fresh water in desalination).

So instead of

nominal_storage_capacity * min_storage_level[t]
    < capacity
    < nominal_storage_capacity * max_storage_level[t]

we would have

nominal_storage_capacity * min_storage_level[t]
    < storage_content
    < nominal_storage_capacity * max_storage_level[t]

This would affect the name in the results api, so we can only change it in a major release.

What do you think?

p-snft commented 4 years ago

Storage capacity is a term used for the energy content of power plant energy storage system.

Capacity, however, typically means a charge ([Q] = 1 C = 1 Ah). It is used, e.g. for batteries and capacitors. For batteries that have a rated voltage (U = const.), it can be used interchangeable with the stored energy. For capacitors, a different word would be preferable. Energy content would be clear and distinct but typically, we do not name the physical quantity (no "power", "energy", or "voltage".). So Storage content would fit better.

PS: I would suggest to add a general glossary to the documentation. (Or stick to the wording according to some clear external definitions. Does there exist a list, e.g. at openmod?)

jnnr commented 4 years ago

Does there exist a list, e.g. at openmod?

There is a glossary at the openmod wiki which has an entry for Battery capacity.

Storage capacity is a term used for the energy content of power plant energy storage system.

The wikipedia definition leaves some room for interpretation, as it does not distinguish possible storage content and actual content at a given time. The openmod definition is more clear:

"The battery capacity is the maximum amount of energy that can be removed from the battery under certain conditions. "

I would suggest to add a general glossary to the documentation.

I like the idea!

ckaldemeyer commented 4 years ago

PS: I would suggest to add a general glossary to the documentation. (Or stick to the wording according to some clear external definitions. Does there exist a list, e.g. at openmod?)

I like the idea, too.

. So Storage content would fit better.

I agree.

jnnr commented 4 years ago

As nobody objected for some time, we can call this a decision. The variable capacity will be renamed storage_content. The proposed API change can be implemented within the v0.4.0 release.

jnnr commented 4 years ago

@joroeder, you remarked that storage_content could cause confusion, as it could be interpreted as oil, water, ... whatever is in the storage. I understand the concern. As a reply: This I would call storage medium. In my eyes, the risk of confusion is little.

However, as GenericStorage should be as generic as possible, it can even represent storages that do not store energy (like a fresh water storage). That's why we came to the conclusion that storage_content would be a good fit (see the discussion above), as it can be interpreted as energy content in case of energy storage but any other content for any kind of storage.

FranziPl commented 4 years ago

Why don't we use state_of_charge instead of storage_content?

uvchik commented 4 years ago

Why don't we use state_of_charge instead of storage_content?

It is always difficult to find the wording for a generic component. As I know it SoC is already a defined as the percentage of the full state and mostly used for batteries or similar storages. I never heard that for a thermal storage or a PHES. Therefore, in my opinion it is better to use a more or less unusual word. If people stumble over the name the may read the docs, if they think that they know what it is they may not read the docs and get it wrong.

jnnr commented 4 years ago

State of charge is a relative quantity normalized to nominal_storage_capacity. We want to describe an absolute quantity (with units of MWh, liters of water, kg of wood, etc.).

FranziPl commented 4 years ago

Ok, I agree to both of you.

joroeder commented 4 years ago

Hm ... yes, it is difficult to find suitable names for generic components and the discussion might get annoying after a while. You are right, SoC is relativ and 'our' storage_content/state_of_charge is absolute. @p-snft introduced the 'inital_storage_level', which is a very nice solution. But the use of 'storage_level' indicates a relative value as well if you want to be consistent to 'initial_storage_level'.

So, we could say 'absolute_storage_level' or 'storage_level_abs'. Or we could divide the equation by nominal_capacity and finally use ‘storage_level’? :D From my perspective 'storage_level' is more intuitive and very general and the documentation would provide the hint that it is an absolute value – just to add some last thoughts on that ;)

In the end, I am fine with ‘storage_content’, since names are just names and you get used to them ;)

As nobody objected for some time, we can call this a decision. The variable capacity will be renamed storage_content. The proposed API change can be implemented within the v0.4.0 release.

Sorry for starting this discussion again, but this might reflect future confusion and complains, and there is some time left unit v0.4.0.

p-snft commented 4 years ago

I'd be fine with storage_content. As level means a normalized quantity ,absolute_storage_level does not fit into the above scheme.

jnnr commented 4 years ago

I promised to have a look at how other frameworks name it. PyPSA calls it state_of_charge, which is kind of confusing, but it has the convention to append _pu (per unit) to relative quantitites.

Calliope just calls it storage, which can be translated 'Einlagerung'.

If no strongly objecting native speaker appears, I am still fine with storage_content.

joroeder commented 4 years ago

okay, this is done, #681 is merged, I will close this PR.