Conversation
…lectricity_source. Style check via new black version is done.
|
@FJanssen-TNO Ready for review |
FJanssen-TNO
left a comment
There was a problem hiding this comment.
I haven't checked every thing yet, but here are some pointers
tests/models/unit_cases_electricity/source_sink_cable/src/example.py
Outdated
Show resolved
Hide resolved
| for source in optimization_problem.energy_system_components.get("electricity_source", []): | ||
| if "PV_without_profile" in source: | ||
| sum = optimization_problem.state(f"{source}.Electricity_source") |
There was a problem hiding this comment.
if you only want electricity production of pv added to the function, then you should only loop over over the pv asset.:
optimization_problem.energy_system_components.get("solarpv", [])
There was a problem hiding this comment.
Now the test is modified. There is no PV w/o profile constraint (but instead Electricity Producer). Hence, I only want electricity production of "Electricity Producer" added to the function. "Electricity Producer" has no component_subtype. Hence, I still keep if condition (if "ElectricityProducer_edde" in source:) after the for loop.
There was a problem hiding this comment.
This for loop over all e-producers, but one with a positive and the other a negative sum is confusing. You could just loop over the e-production assets and only add to the sum if the asset is not solar_pv. Either solar_pv is a subgroup and you can check if it is not in that or the solarpv asset is already not included in this electricity_source group.
There was a problem hiding this comment.
Sorry. my previous comment was outdated (I forgot to update that comment after I have modified the objective function. Hence, that comment does not explain what I did back then). Back then I actually intentionally made contribution of pv to objective function negative and electric generator positive (in that moment I thought it might be better to understand the aim of the objective function). But actually only using electric generator was also enough. Now, I made a new update in the objective function, i only take into account electricity producer in that part of the objective function.
| np.testing.assert_allclose( | ||
| results["PV__max_size"], | ||
| max(solution.get_timeseries("PV.maximum_electricity_source").values[1:]), | ||
| ) |
There was a problem hiding this comment.
I actually want to see that it can scale the profile.
There was a problem hiding this comment.
I updated the objective function in a way that we ensure that constraint profile sizing in asset_sizing works as expected
There was a problem hiding this comment.
I'm missing a check that the PV is not sized to its max size, but smaller.
tests/models/unit_cases_electricity/source_sink_cable/src/example.py
Outdated
Show resolved
Hide resolved
src/mesido/asset_sizing_mixin.py
Outdated
| (np_ones * max_power - electricity_source) / constraint_nominal, | ||
| 0.0, | ||
| np.inf, | ||
| if (d in self.energy_system_components.get("solar_pv", [])) and ( |
There was a problem hiding this comment.
Do we only want to allow this in case the asset is a solar_pv?
There was a problem hiding this comment.
Since this PR was only related to solar_pv, I intentionally did not want to allow all electricity producers (other component subtypes of electricity_source). Otherwise, I would needed to add tests for the other electricity producers with profile constraints. If you like I can remove solar_pv condition and make this sizing valid for all electricity producers.
|
@FJanssen-TNO Ready for review |
|
@FJanssen-TNO ready for the review |
tests/models/unit_cases_electricity/source_sink_cable/src/example.py
Outdated
Show resolved
Hide resolved
| for source in self.energy_system_components["electricity_source"]: | ||
| goals.append(MinimizeElecProductionSize(source, total_demand)) |
There was a problem hiding this comment.
The total_demand seems unnecessary. You don't need a target here.
This goal should even not be a path goal but a normal goal. So you should move it to
def goals()
There was a problem hiding this comment.
Yes. I agree. Constraint on max_size should not be done via path_goals() but goals().
| class MinimizeElecProductionSize(Goal): | ||
| priority = 2 | ||
|
|
||
| order = 2 | ||
|
|
||
| def __init__(self, source, total_demand): | ||
| self.source = source | ||
| self.target_min = total_demand | ||
| self.target_max = total_demand | ||
| self.function_range = (0.0, 2.0 * max(total_demand.values)) | ||
| self.function_nominal = np.median(total_demand.values) | ||
|
|
||
| def function(self, optimization_problem, ensemble_member): | ||
| return optimization_problem.state(f"{self.source}__max_size") * 2.0 |
There was a problem hiding this comment.
This should be a "normal goal". No target is assigned for sizing variable.
Furthermore in the function itself you also multiply it with "2" what is the reason for that?
f"{self.source}__max_size" is not state variable, it is a variable that only exists once. You have to extract it iusing optimization_problem.extra_variable()
| if total_demand is None: | ||
| total_demand = target.copy() if hasattr(target, "copy") else target | ||
| else: | ||
| total_demand += target |
There was a problem hiding this comment.
The total_demand is unnecessary.
There was a problem hiding this comment.
I removed the total_demand vector, but I still use maximum of the electricity demand to define self.function_nominal in MinimizeElecProductionSize.
| class _GoalsAndOptionsPV: | ||
| def path_goals(self): | ||
| """ | ||
| Add goal to meet the specified power demands in the electricity network. | ||
|
|
||
| Returns | ||
| ------- | ||
| Extended goals list. | ||
| """ | ||
| goals = super().path_goals().copy() |
There was a problem hiding this comment.
Instead of another _GoalsAndOptions, check what can already be placed in the orginal one and else add them to the problem directly instead of creating a new class, there is no need for that right now.
There was a problem hiding this comment.
Good idea. Makes it cleaner.
| np.testing.assert_allclose( | ||
| results["PV__max_size"], | ||
| max(solution.get_timeseries("PV.maximum_electricity_source").values[1:]), | ||
| results["PV.Electricity_source"], profile_scaled * results["PV__max_size"] |
There was a problem hiding this comment.
Some checks are still missing like, check that the max PV size is actually the same as the max demand and thus much smaller than the optional size.
Also this check shouldn't be an all close but smaller than.
src/mesido/asset_sizing_mixin.py
Outdated
There was a problem hiding this comment.
This entire if/else statement could be placed in a generic function instead of only the cap of the profile. All relevant information is already pass on.
# Conflicts: # src/mesido/asset_sizing_mixin.py
| self.source = source | ||
| self.target_min = 1e-6 | ||
| self.function_range = (0.0, 2.0 * nominal) | ||
| self.function_nominal = nominal |
There was a problem hiding this comment.
function_nominal has to be the same for all assets --> as otherwise the one assetsize is scaled larger/smaller than the other, thereby the optimization is not exactly on their size. If the sizes are in the magnitude of MW, then the function_nominal could be set to 1e6.
There was a problem hiding this comment.
now function_nominal is defined same for all assets and equal to 1e6
| def __init__(self, source, nominal): | ||
| self.source = source | ||
| self.target_min = 1e-6 | ||
| self.function_range = (0.0, 2.0 * nominal) |
There was a problem hiding this comment.
function_range is typically set to 2*max value.
There was a problem hiding this comment.
now it is nor nominal but max_value
| if source not in self.energy_system_components.get("solar_pv", []): | ||
| nominal = float(self.bounds()[f"{source}.Electricity_source"][1]) / 2.0 | ||
| else: | ||
| nominal = max(self.bounds()[f"{source}.Electricity_source"][1].values) / 2.0 |
There was a problem hiding this comment.
the information you pass on can be:
| if source not in self.energy_system_components.get("solar_pv", []): | |
| nominal = float(self.bounds()[f"{source}.Electricity_source"][1]) / 2.0 | |
| else: | |
| nominal = max(self.bounds()[f"{source}.Electricity_source"][1].values) / 2.0 | |
| max_val = self.bounds()[f"{source}.Electricity_source"][1] |
which is indep of asset type.
There was a problem hiding this comment.
now I call the max_value. but I still keep if/else conditions. Because, on electricty producer has a time Reries as upper bound other has just a scalar
src/mesido/asset_sizing_mixin.py
Outdated
| max_profile_non_scaled = max(profile_non_scaled) | ||
| profile_scaled = profile_non_scaled / max_profile_non_scaled | ||
|
|
||
| # Cap the electricity produced via a profile. Two profile options below. |
There was a problem hiding this comment.
| # Cap the electricity produced via a profile. Two profile options below. | |
| # Cap the energy produced via a profile. Two profile options below. |
There was a problem hiding this comment.
word is replaced
src/mesido/asset_sizing_mixin.py
Outdated
| self, | ||
| asset, | ||
| variable_suffix, # i.e. maximum_electricity_source, maximum_heat_source | ||
| source, # i.e. heat_source, energy source | ||
| max_power, # i.e. max_heat, max_power | ||
| constraint_nominal, | ||
| ensemble_member, |
There was a problem hiding this comment.
Typehinting should be added here.
There was a problem hiding this comment.
typehinting is now done as how you explained in the comment
| """ | ||
| Add goal to minimize max_size of electricity producers while ensuring | ||
| that they are equal to each other. | ||
|
|
||
| Returns | ||
| ------- | ||
| Extended goals list. | ||
| """ |
There was a problem hiding this comment.
comments from these goals are removed
tests/test_electric_source_sink.py
Outdated
| ) | ||
| results = solution.extract_results() | ||
|
|
||
| # Test demand matching |
There was a problem hiding this comment.
This comment is unnecessary, the demand_matching_test name is already describing it. same for electric_power_conservation_test.
There was a problem hiding this comment.
comment is removed
|
|
||
| def __init__(self, source, nominal): | ||
| self.source = source | ||
| self.target_min = 1e-6 |
There was a problem hiding this comment.
why did you put in this target_min?
There was a problem hiding this comment.
order = 2 required to pass a target_min or target_max. I thought max value of self.function_range should be equal to self.target_max but as I defined both equal to each other I got error indicating that self.target_max should be smllaer than the upper bound of self.function_range. Hence, I decided to not to define self.target_max but self.target_min. But now I define self.target_max = max_value, whereas upper bound of self.function_range as 2*max_value. Hence, this is better now
| np.testing.assert_allclose( | ||
| solution.get_timeseries("ElectricityDemand_2af6.target_electricity_demand").values, | ||
| results["ElectricityProducer_edde.Electricity_source"] | ||
| + results["PV.Electricity_source"], | ||
| ) |
There was a problem hiding this comment.
This check is unnecessary as it is already part of the electrical_power_conservation_test
| np.testing.assert_allclose( | ||
| results["PV__max_size"], | ||
| max(solution.get_timeseries("PV.maximum_electricity_source").values[1:]), | ||
| ) |
There was a problem hiding this comment.
I'm missing a check that the PV is not sized to its max size, but smaller.
|
@FJanssen-TNO I did the modification based on PR review. But, still I have issue with the problem formulation. The objective functions do not constraint the the problem in a way we want. For instance, I used "MinimizeElecProductionSize" to make PV max size equal to Electricity producer max size. But it does not work. results["ElectricityProducer_edde__max_size"] is not equal to max(results["ElectricityProducer_edde.Electricity_source"]). Also other checks that I placed inthe test does not pass. Can you have a look at the branch? |
DONE: