Review/897/distinguish fallback value and max value by Flix6x · Pull Request #909 · FlexMeasures/flexmeasures

Flix6x · 2023-11-23T20:16:10Z

This is the second part of my post-review work, dealing with the distinction between:

falling back on a db model attribute in case something isn't defined in the flex-model, and
making sure consumption/production capacities do not exceed a given power capacity.

I split this work into two commits, best considered separately:

49a155a deals with the distinction explained above, and
c1b24dd deals specifically with the case of having gaps in the sensor data of the production/consumption capacity.

In the second commit, I'm proposing to fill these with the power capacity, rather than with the value coming from the fallback attribute. I realize I am proposing this with the consideration of obtaining a consistent policy throughout our API, more so than practical considerations for a given use case. That API policy being that db model attributes are only used as fallbacks for missing fields, rather than as limitations on fields, of flex-models sent through the API.

I'm open to discussing potential merits of deviating from this policy, of course, perhaps by hands of an example?

Signed-off-by: F.N. Claessen <[email protected]>

…ing a maximum capacity limit Signed-off-by: F.N. Claessen <[email protected]>

…n with the fallback Signed-off-by: F.N. Claessen <[email protected]>

victorgarcia98

Nice!

I agree, it's a good policy to have just one fallback and not multiple through chained attributes that have a fallback attribute). Even though in some settings might be interesting, It's better to have a consistent/expected behavior.

For example, in the case of a site power capacity, typically the consumption/production capacities are the same, thus, we can default the directional capacities to the global capacity. This is happening

flexmeasures/flexmeasures/data/models/planning/storage.py

Lines 285 to 330 in c1b24dd

    
                   ems_consumption_capacity_in_mw = self.flex_context.get( 
        
                       "ems_consumption_capacity_in_mw", 
        
                       self.sensor.generic_asset.get_attribute( 
        
                           "consumption_capacity_in_mw", np.nan 
        
                       ), 
        
                   ) 
        
                   """ 
        
                   Priority order to fetch the site production power capacity: 
        
                   "site-production-capacity" (flex-context) -> "production_capacity_in_mw" (asset attribute) 
        
                   where the flex-context is in its serialized form. 
        
                   """ 
        
                   ems_production_capacity_in_mw = self.flex_context.get( 
        
                       "ems_production_capacity_in_mw", 
        
                       self.sensor.generic_asset.get_attribute( 
        
                           "production_capacity_in_mw", np.nan 
        
                       ), 
        
                   ) 
        
                   if not np.isnan(ems_consumption_capacity_in_mw): 
        
                       assert ( 
        
                           ems_consumption_capacity_in_mw >= 0 
        
                       ), "EMS consumption capacity needs to be nonnegative." 
        
                       ems_consumption_capacity_in_mw = check_and_convert_power_capacity( 
        
                           ems_consumption_capacity_in_mw 
        
                       ) 
        
                   if not np.isnan(ems_production_capacity_in_mw): 
        
                       assert ( 
        
                           ems_production_capacity_in_mw >= 0 
        
                       ), "EMS production capacity needs to be nonnegative." 
        
                       ems_production_capacity_in_mw = check_and_convert_power_capacity( 
        
                           ems_production_capacity_in_mw 
        
                       ) 
        
                   else: 
        
                       ems_production_capacity_in_mw = np.nan 
        
                   ems_constraints["derivative min"] = -np.nanmin( 
        
                       [ems_production_capacity_in_mw, capacity_in_mw] 
        
                   ) 
        
                   ems_constraints["derivative max"] = np.nanmin( 
        
                       [ems_consumption_capacity_in_mw, capacity_in_mw] 
        
                   )

…apacity-as-sensor' into review/897/distinguish-default-value-and-max-value

…ml#typing.Optional) Signed-off-by: F.N. Claessen <[email protected]>

Signed-off-by: F.N. Claessen <[email protected]>

…tity, as its name already suggested Signed-off-by: F.N. Claessen <[email protected]>

Signed-off-by: F.N. Claessen <[email protected]>

Flix6x · 2023-11-27T12:37:09Z

I refactored a bit further, which made for a much slimmer get_continuous_series_sensor_or_quantity method. Two more ideas, which I'll leave to you:

Renaming said method.
Looking up the fallback attribute already when deserializing the flex model. I believe that would make sense in conjunction with Migrate attribute names to use dashed to separate words #911, though it could/should be a separate ticket. That is, the flex-model field and db model attribute would both represent a serialized object and both be called "consumption-capacity", and the db model attribute is used as a fallback already before deserialization.

victorgarcia98 · 2023-11-27T16:04:54Z

* Renaming said method.

Perhaps, we can make the function a static method of the class QuantityOrSensor and rename it to get_series, for instance. What do you think? It would look something like

QuantityOrSensor.get_series(quantity_or_sensor, start, end, ...)

However, this looks the same size if not more.

An alternative name could be get_regular_ts(...).

* Looking up the fallback attribute already when deserializing the flex model. I believe that would make sense in conjunction with [Migrate attribute names to use dashed to separate words #911](https://github.com/FlexMeasures/flexmeasures/issues/911), though it could/should be a separate ticket. That is, the flex-model field and db model attribute would both represent a serialized object and both be called `"consumption-capacity"`, and the db model attribute is used as a fallback _already before_ deserialization.

Good idea! This is a feature that we are going to find a lot and it's better to bring the functionality into the marshmallow field.

Flix6x · 2023-11-27T23:03:42Z

I'm expecting that moving the fallback logic into the schema will also lead to more clarity on choosing a new name for the util method. Maybe tackle those together.

* add (device) consumption capacity and production capacity as sensors Signed-off-by: Victor Garcia Reolid <[email protected]> * convert units of the power limits to the sensor units Signed-off-by: Victor Garcia Reolid <[email protected]> * add QuantityOrSensor field Signed-off-by: Victor Garcia Reolid <[email protected]> * add annotations Signed-off-by: Victor Garcia Reolid <[email protected]> * introduce QuantityorSensor schema Signed-off-by: Victor Garcia Reolid <[email protected]> * add from __future__ import annotations Signed-off-by: Victor Garcia Reolid <[email protected]> * remove discarded revision Signed-off-by: Victor Garcia Reolid <[email protected]> * add target unit Signed-off-by: Victor Garcia Reolid <[email protected]> * change exception type to FMValidationError Signed-off-by: Victor Garcia Reolid <[email protected]> * add explicity default value Signed-off-by: Victor Garcia Reolid <[email protected]> * split into two functions Signed-off-by: Victor Garcia Reolid <[email protected]> * Review/897/refactoring (#908) * refactor: fix spelling Signed-off-by: F.N. Claessen <[email protected]> * style: missing type annotation Signed-off-by: F.N. Claessen <[email protected]> * refactor: method and variable renaming Signed-off-by: F.N. Claessen <[email protected]> * refactor: add test case explanations Signed-off-by: F.N. Claessen <[email protected]> --------- Signed-off-by: F.N. Claessen <[email protected]> * Review/897/distinguish fallback value and max value (#909) * refactor: fix spelling Signed-off-by: F.N. Claessen <[email protected]> * style: missing type annotation Signed-off-by: F.N. Claessen <[email protected]> * refactor: method and variable renaming Signed-off-by: F.N. Claessen <[email protected]> * refactor: add test case explanations Signed-off-by: F.N. Claessen <[email protected]> * fix: separate logic for falling back on a default attribute and applying a maximum capacity limit Signed-off-by: F.N. Claessen <[email protected]> * fix: fill gaps in capacity sensor data using the max_value rather than with the fallback Signed-off-by: F.N. Claessen <[email protected]> * fix: type annotation (see https://docs.python.org/3/library/typing.html#typing.Optional) Signed-off-by: F.N. Claessen <[email protected]> * refactor: clearly separate steps of falling back and performing a nanmin Signed-off-by: F.N. Claessen <[email protected]> * refactor: simplify and have get_quantity_from_attribute return a Quantity, as its name already suggested Signed-off-by: F.N. Claessen <[email protected]> * refactor: switch argument order Signed-off-by: F.N. Claessen <[email protected]> * refactor: update docstring Signed-off-by: F.N. Claessen <[email protected]> * style: flake8 Signed-off-by: F.N. Claessen <[email protected]> --------- Signed-off-by: F.N. Claessen <[email protected]> * fix test Signed-off-by: Victor Garcia Reolid <[email protected]> * add changelog entry Signed-off-by: Victor Garcia Reolid <[email protected]> * update changelog Signed-off-by: Victor Garcia Reolid <[email protected]> --------- Signed-off-by: Victor Garcia Reolid <[email protected]> Signed-off-by: F.N. Claessen <[email protected]> Co-authored-by: Felix Claessen <[email protected]>

* add (device) consumption capacity and production capacity as sensors Signed-off-by: Victor Garcia Reolid <[email protected]> * convert units of the power limits to the sensor units Signed-off-by: Victor Garcia Reolid <[email protected]> * add QuantityOrSensor field Signed-off-by: Victor Garcia Reolid <[email protected]> * add annotations Signed-off-by: Victor Garcia Reolid <[email protected]> * introduce QuantityorSensor schema Signed-off-by: Victor Garcia Reolid <[email protected]> * add from __future__ import annotations Signed-off-by: Victor Garcia Reolid <[email protected]> * remove discarded revision Signed-off-by: Victor Garcia Reolid <[email protected]> * add target unit Signed-off-by: Victor Garcia Reolid <[email protected]> * change exception type to FMValidationError Signed-off-by: Victor Garcia Reolid <[email protected]> * add explicity default value Signed-off-by: Victor Garcia Reolid <[email protected]> * split into two functions Signed-off-by: Victor Garcia Reolid <[email protected]> * add usage forecast Signed-off-by: Victor Garcia Reolid <[email protected]> * add test Signed-off-by: Victor Garcia Reolid <[email protected]> * Review/897/refactoring (#908) * refactor: fix spelling Signed-off-by: F.N. Claessen <[email protected]> * style: missing type annotation Signed-off-by: F.N. Claessen <[email protected]> * refactor: method and variable renaming Signed-off-by: F.N. Claessen <[email protected]> * refactor: add test case explanations Signed-off-by: F.N. Claessen <[email protected]> --------- Signed-off-by: F.N. Claessen <[email protected]> * use resolution to convert energy series to the target event resolution Signed-off-by: Victor Garcia Reolid <[email protected]> * simplify conftest Signed-off-by: Victor Garcia Reolid <[email protected]> * Review/897/distinguish fallback value and max value (#909) * refactor: fix spelling Signed-off-by: F.N. Claessen <[email protected]> * style: missing type annotation Signed-off-by: F.N. Claessen <[email protected]> * refactor: method and variable renaming Signed-off-by: F.N. Claessen <[email protected]> * refactor: add test case explanations Signed-off-by: F.N. Claessen <[email protected]> * fix: separate logic for falling back on a default attribute and applying a maximum capacity limit Signed-off-by: F.N. Claessen <[email protected]> * fix: fill gaps in capacity sensor data using the max_value rather than with the fallback Signed-off-by: F.N. Claessen <[email protected]> * fix: type annotation (see https://docs.python.org/3/library/typing.html#typing.Optional) Signed-off-by: F.N. Claessen <[email protected]> * refactor: clearly separate steps of falling back and performing a nanmin Signed-off-by: F.N. Claessen <[email protected]> * refactor: simplify and have get_quantity_from_attribute return a Quantity, as its name already suggested Signed-off-by: F.N. Claessen <[email protected]> * refactor: switch argument order Signed-off-by: F.N. Claessen <[email protected]> * refactor: update docstring Signed-off-by: F.N. Claessen <[email protected]> * style: flake8 Signed-off-by: F.N. Claessen <[email protected]> --------- Signed-off-by: F.N. Claessen <[email protected]> * fix test Signed-off-by: Victor Garcia Reolid <[email protected]> * add changelog entry Signed-off-by: Victor Garcia Reolid <[email protected]> * improve tests and implement consumption_is_positive Signed-off-by: Victor Garcia Reolid <[email protected]> * add resolution factor to convert from energy units to power units Signed-off-by: Victor Garcia Reolid <[email protected]> * fix test + add new case Signed-off-by: Victor Garcia Reolid <[email protected]> * split stock delta into stock gain and stock usage Signed-off-by: Victor Garcia Reolid <[email protected]> * update changelog Signed-off-by: Victor Garcia Reolid <[email protected]> * switch to power units Signed-off-by: Victor Garcia Reolid <[email protected]> * improve comments Signed-off-by: Victor Garcia Reolid <[email protected]> * add changelog Signed-off-by: Victor Garcia Reolid <[email protected]> * Update changelog.rst Fix typo (*los componentes*) and swap out some changelog entries between Features and Infrastructure. Signed-off-by: Felix Claessen <[email protected]> * Apply black Signed-off-by: Victor Garcia Reolid <[email protected]> --------- Signed-off-by: Victor Garcia Reolid <[email protected]> Signed-off-by: F.N. Claessen <[email protected]> Signed-off-by: Victor <[email protected]> Signed-off-by: Felix Claessen <[email protected]> Co-authored-by: Felix Claessen <[email protected]>

* add (device) consumption capacity and production capacity as sensors Signed-off-by: Victor Garcia Reolid <[email protected]> * convert units of the power limits to the sensor units Signed-off-by: Victor Garcia Reolid <[email protected]> * add QuantityOrSensor field Signed-off-by: Victor Garcia Reolid <[email protected]> * add annotations Signed-off-by: Victor Garcia Reolid <[email protected]> * introduce QuantityorSensor schema Signed-off-by: Victor Garcia Reolid <[email protected]> * add from __future__ import annotations Signed-off-by: Victor Garcia Reolid <[email protected]> * remove discarded revision Signed-off-by: Victor Garcia Reolid <[email protected]> * add target unit Signed-off-by: Victor Garcia Reolid <[email protected]> * change exception type to FMValidationError Signed-off-by: Victor Garcia Reolid <[email protected]> * add explicity default value Signed-off-by: Victor Garcia Reolid <[email protected]> * split into two functions Signed-off-by: Victor Garcia Reolid <[email protected]> * Review/897/refactoring (#908) * refactor: fix spelling Signed-off-by: F.N. Claessen <[email protected]> * style: missing type annotation Signed-off-by: F.N. Claessen <[email protected]> * refactor: method and variable renaming Signed-off-by: F.N. Claessen <[email protected]> * refactor: add test case explanations Signed-off-by: F.N. Claessen <[email protected]> --------- Signed-off-by: F.N. Claessen <[email protected]> * Review/897/distinguish fallback value and max value (#909) * refactor: fix spelling Signed-off-by: F.N. Claessen <[email protected]> * style: missing type annotation Signed-off-by: F.N. Claessen <[email protected]> * refactor: method and variable renaming Signed-off-by: F.N. Claessen <[email protected]> * refactor: add test case explanations Signed-off-by: F.N. Claessen <[email protected]> * fix: separate logic for falling back on a default attribute and applying a maximum capacity limit Signed-off-by: F.N. Claessen <[email protected]> * fix: fill gaps in capacity sensor data using the max_value rather than with the fallback Signed-off-by: F.N. Claessen <[email protected]> * fix: type annotation (see https://docs.python.org/3/library/typing.html#typing.Optional) Signed-off-by: F.N. Claessen <[email protected]> * refactor: clearly separate steps of falling back and performing a nanmin Signed-off-by: F.N. Claessen <[email protected]> * refactor: simplify and have get_quantity_from_attribute return a Quantity, as its name already suggested Signed-off-by: F.N. Claessen <[email protected]> * refactor: switch argument order Signed-off-by: F.N. Claessen <[email protected]> * refactor: update docstring Signed-off-by: F.N. Claessen <[email protected]> * style: flake8 Signed-off-by: F.N. Claessen <[email protected]> --------- Signed-off-by: F.N. Claessen <[email protected]> * fix test Signed-off-by: Victor Garcia Reolid <[email protected]> * add changelog entry Signed-off-by: Victor Garcia Reolid <[email protected]> * update changelog Signed-off-by: Victor Garcia Reolid <[email protected]> * add charge and discharge efficiencies Signed-off-by: Victor Garcia Reolid <[email protected]> * fix test Signed-off-by: Victor Garcia Reolid <[email protected]> * add extra validation and use 100 for the missing values instead of the roundtrip Signed-off-by: Victor Garcia Reolid <[email protected]> * fix validation rules Signed-off-by: Victor Garcia Reolid <[email protected]> * check roundtrip efficiency Signed-off-by: Victor Garcia Reolid <[email protected]> * add documentation Signed-off-by: Victor Garcia Reolid <[email protected]> --------- Signed-off-by: Victor Garcia Reolid <[email protected]> Signed-off-by: F.N. Claessen <[email protected]> Co-authored-by: Felix Claessen <[email protected]>

Flix6x added 6 commits November 21, 2023 11:33

refactor: fix spelling

a885b7d

Signed-off-by: F.N. Claessen <[email protected]>

style: missing type annotation

8af4113

Signed-off-by: F.N. Claessen <[email protected]>

refactor: method and variable renaming

cd54f04

Signed-off-by: F.N. Claessen <[email protected]>

refactor: add test case explanations

fe21a28

Signed-off-by: F.N. Claessen <[email protected]>

fix: separate logic for falling back on a default attribute and apply…

49a155a

…ing a maximum capacity limit Signed-off-by: F.N. Claessen <[email protected]>

fix: fill gaps in capacity sensor data using the max_value rather tha…

c1b24dd

…n with the fallback Signed-off-by: F.N. Claessen <[email protected]>

Flix6x added the Scheduling label Nov 23, 2023

Flix6x requested a review from victorgarcia98 November 23, 2023 20:16

Flix6x self-assigned this Nov 23, 2023

Base automatically changed from review/897/refactoring to feature/planning/battery-power-capacity-as-sensor November 24, 2023 08:54

victorgarcia98 approved these changes Nov 24, 2023

View reviewed changes

Flix6x added 7 commits November 26, 2023 10:26

Merge remote-tracking branch 'origin/feature/planning/battery-power-c…

8a1df28

…apacity-as-sensor' into review/897/distinguish-default-value-and-max-value

fix: type annotation (see https://docs.python.org/3/library/typing.ht…

aea6f4a

…ml#typing.Optional) Signed-off-by: F.N. Claessen <[email protected]>

refactor: clearly separate steps of falling back and performing a nanmin

67dd431

Signed-off-by: F.N. Claessen <[email protected]>

refactor: simplify and have get_quantity_from_attribute return a Quan…

2e551d6

…tity, as its name already suggested Signed-off-by: F.N. Claessen <[email protected]>

refactor: switch argument order

9c6a446

Signed-off-by: F.N. Claessen <[email protected]>

refactor: update docstring

891054b

Signed-off-by: F.N. Claessen <[email protected]>

style: flake8

38718ea

Signed-off-by: F.N. Claessen <[email protected]>

Flix6x merged commit 9fadfcc into feature/planning/battery-power-capacity-as-sensor Nov 27, 2023

Flix6x deleted the review/897/distinguish-default-value-and-max-value branch November 27, 2023 12:37

victorgarcia98 mentioned this pull request Nov 27, 2023

Implement fallback attribute mechanism in the flex-model load step #914

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review/897/distinguish fallback value and max value#909

Review/897/distinguish fallback value and max value#909
Flix6x merged 13 commits intofeature/planning/battery-power-capacity-as-sensorfrom
review/897/distinguish-default-value-and-max-value

Flix6x commented Nov 23, 2023

Uh oh!

victorgarcia98 left a comment

Uh oh!

Flix6x commented Nov 27, 2023

Uh oh!

victorgarcia98 commented Nov 27, 2023

Uh oh!

Flix6x commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	ems_consumption_capacity_in_mw = self.flex_context.get(
	"ems_consumption_capacity_in_mw",
	self.sensor.generic_asset.get_attribute(
	"consumption_capacity_in_mw", np.nan
	),
	)

	"""
	Priority order to fetch the site production power capacity:

	"site-production-capacity" (flex-context) -> "production_capacity_in_mw" (asset attribute)

	where the flex-context is in its serialized form.
	"""
	ems_production_capacity_in_mw = self.flex_context.get(
	"ems_production_capacity_in_mw",
	self.sensor.generic_asset.get_attribute(
	"production_capacity_in_mw", np.nan
	),
	)

	if not np.isnan(ems_consumption_capacity_in_mw):
	assert (
	ems_consumption_capacity_in_mw >= 0
	), "EMS consumption capacity needs to be nonnegative."

	ems_consumption_capacity_in_mw = check_and_convert_power_capacity(
	ems_consumption_capacity_in_mw
	)

	if not np.isnan(ems_production_capacity_in_mw):
	assert (
	ems_production_capacity_in_mw >= 0
	), "EMS production capacity needs to be nonnegative."
	ems_production_capacity_in_mw = check_and_convert_power_capacity(
	ems_production_capacity_in_mw
	)
	else:
	ems_production_capacity_in_mw = np.nan

	ems_constraints["derivative min"] = -np.nanmin(
	[ems_production_capacity_in_mw, capacity_in_mw]
	)
	ems_constraints["derivative max"] = np.nanmin(
	[ems_consumption_capacity_in_mw, capacity_in_mw]
	)

Conversation

Flix6x commented Nov 23, 2023

Uh oh!

victorgarcia98 left a comment

Choose a reason for hiding this comment

Uh oh!

Flix6x commented Nov 27, 2023

Uh oh!

victorgarcia98 commented Nov 27, 2023

Uh oh!

Flix6x commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants