quintel / etmodel

Professional interface of the Energy Transition model.
https://energytransitionmodel.com/
MIT License
33 stars 14 forks source link

Updat statements for BOTH don't (always?) work with shares and share groups #2324

Closed ChaelKruip closed 7 years ago

ChaelKruip commented 7 years ago

@AlexanderWirtz told me that setting shares for the space_heating share-group in households using update statements which apply to "both" results in a scenario where the sliders become unresponsive.

@ploh you can test this by using the update statements like this one. Note that every member of the groups needs to get a value and that shares should add up to 100% (1.0).

AlexanderWirtz commented 7 years ago

@AlexanderWirtz told me that setting shares for the space_heating share-group in households using update statements which apply to "both" results in a scenario where the sliders become unresponsive.

It is tricky. Something does happen, but not what you want. setting the space_heating shares separately for present and future on the other hand, works just like you want it to.

ploh commented 7 years ago

@ChaelKruip You mentioned an old issue/discussion about this. Have you found that? @antw Do you happen to know which old issue that is?

ChaelKruip commented 7 years ago

@ChaelKruip You mentioned an old issue/discussion about this. Have you found that?

No 😢 . I think @AlexanderWirtz had some Skype conversations with @antw with this but did not make tickets.

grdw commented 7 years ago

screen shot 2017-01-16 at 17 52 51

These are the values from initially loading the shares. These are my initializer inputs:

screen shot 2017-01-16 at 17 53 35

That corresponds I guess.

I have no idea how to validate this in ETEngine though 🤔

grdw commented 7 years ago

@AlexanderWirtz dragging a slider doesn't cause any weird behavior as far as I can see.

screen shot 2017-01-16 at 17 58 13

antw commented 7 years ago

I think @AlexanderWirtz had some Skype conversations with @antw with this but did not make tickets.

We haven't had a Skype call since 2014! 😱 Maybe we discussed this in person at some point, but I have no memory of this at all. Sorry, I know that isn't helpful...

Perhaps this issue is referring to quintel/etengine#808? This was because inputs which updated the present graph were (wrongly) assigned to a group of inputs which updated future, and so the group didn't add up to 100%. It goes without saying that you cannot mix-and-match grouped inputs that update present or future with inputs that update both.

However, you shouldn't have to mix present/future with both when using initializer inputs; init. inputs will update present and future graphs (without the need for present or both inputs), and then you can use the normal future inputs just as you do in a normal scenario.

ploh commented 7 years ago

Perhaps this issue is referring to quintel/etengine#808? This was because inputs which updated the present graph were (wrongly) assigned to a group of inputs which updated future, and so the group didn't add up to 100%.

Yes, that is it. Thank you. That confirms my suspicions I had earlier about the share_groups causing problems when I was looking at the ScenarioUpdater :+1:

ChaelKruip commented 7 years ago

That confirms my suspicions I had earlier about the share_groups causing problems when I was looking at the ScenarioUpdater

@grdw did you manage to update a slider group yet (using the right share_group)? If so, we could close this right?

AlexanderWirtz commented 7 years ago

@AlexanderWirtz dragging a slider doesn't cause any weird behavior as far as I can see.

@grdw I can't tell from your screenshot as I would like to see the chart. As I remember, when the scenario is loaded it looks fine (i.e. present and future are the same). Then when you touch a slider and another one, the chart goes haywire.

Perhaps this issue is referring to quintel/etengine#808?

@antw that was a separate issue. As you can see the existing update statements for both for tech shares have their own both share group. Nonetheless, I couldn't get them to work at the time. Really sorry this was never documented properly.

ploh commented 7 years ago

As you can see the existing update statements for both for tech shares have their own both share group. Nonetheless, I couldn't get them to work at the time. Really sorry this was never documented properly.

@AlexanderWirtz I tried to use some of these existing both updates (namely those for the heating_households_both group), i.e. I modified scenario 507792 (one of the GEA scenarios) locally to use the both statements instead of future + present statements. Then I got (one of) the described behaviour: Moving the sliders had no effect on the chart (or the dashboard). After some debugging I found out that it was a simple ordering/priority problem: The both inputs have the same priority as the future inputs (which are used by the sliders). By chance the both inputs are applied last and thus overwrite the future inputs. After fixing the priority (so that both has higher priority and hence comes first), everything works as expected.

grdw commented 7 years ago

After fixing the priority

How did you manage to do this?

I still see this though (the initializer inputs do reflect in the chart but not in the sliders):

screen shot 2017-01-18 at 09 31 10

Related initializer inputs (add-up to a 100):

- init.households_space_heater_coal_share = 10
- init.households_space_heater_combined_network_gas_share = 10
- init.households_space_heater_crude_oil_share = 20
- init.households_space_heater_district_heating_steam_hot_water_share = 10
- init.households_space_heater_electricity_share = 5
- init.households_space_heater_heatpump_air_water_electricity_share = 5
- init.households_space_heater_heatpump_ground_water_electricity_share = 5
- init.households_space_heater_hybrid_heatpump_air_water_electricity_share = 5
- init.households_space_heater_micro_chp_network_gas_share = 5
- init.households_space_heater_network_gas_share = 5
- init.households_space_heater_wood_pellets_share = 20
grdw commented 7 years ago

To add to that:

screen shot 2017-01-18 at 09 33 31

Adding 1% to Heat pump (air) causes the chart to break like the image above.

grdw commented 7 years ago

However, I managed to get the sliders correctly:

screen shot 2017-01-18 at 09 48 52

I did that by removing the update_period from the initializer inputs and changing the share_group from heating_households_both to heating_households. However, now my user_values are not reflected in the chart at all 🤔

ploh commented 7 years ago

After fixing the priority

How did you manage to do this?

With this change to ETEngine - but I could have manually changed the relevant etsource files instead.

diff --git a/app/models/scenario/user_updates.rb b/app/models/scenario/user_updates.rb
index 2ea97671..ab24833f 100644
--- a/app/models/scenario/user_updates.rb
+++ b/app/models/scenario/user_updates.rb
@@ -93,7 +93,7 @@ module Scenario::UserUpdates
   def inputs
     @inputs ||=
       combined_values.map { |key, _| Input.get(key) }.
-      compact.sort_by(&:priority).reverse
+      compact.sort_by { |input| [input.priority, input.update_period == 'both' ? 1 : 0] }.reverse
   end

   # Internal: A hash of inputs, and the values to be set on the named graph.
grdw commented 7 years ago

However, now my user_values are not reflected in the chart at all

screen shot 2017-01-18 at 09 55 21

I also managed to fix that; my mistake. An intializer_input should not have the same name as another input 😅

ploh commented 7 years ago

I also managed to fix that; my mistake. An intializer_input should not have the same name as another input :sweat_smile:

@grdw Does Atlas not raise an error if a key is not unique?

grdw commented 7 years ago

With this change to ETEngine - but I could have manually changed the relevant etsource files instead.

Yes. It's what I currently did. It should not have to do with the priority at all for initializer inputs and slider settings would be my guess (side note: it's a bit annoying that talking about user_values causes me to think about initializer inputs and slider settings). I'm now going to test if removing the priority from the initializer inputs is going to work or just setting them to a really low priority like 0. It should not influence the chart I generated in the above comment.

@grdw Does Atlas not raise an error if a key is not unique?

Eh, nope. Not that I'm aware... ETEngine thinks all is good and continues on working.

ploh commented 7 years ago

It should not have to do with the priority at all for initializer inputs and slider settings would be my guess

The priority is irrelevant: We first apply the init. inputs and then the user_values

ploh commented 7 years ago

@grdw It is all working for you now, right? Or did I misinterpret your last posted chart?

grdw commented 7 years ago

@grdw It is all working for you now, right? Or did I misinterpret your last posted chart?

Yes it is! 👍

grdw commented 7 years ago

@AlexanderWirtz did a test that did break the chart, namely: "hitting the rest button next to a slider".

That broke the chart again:

Load initializer inputs: screen shot 2017-01-18 at 10 11 49

Update user value: screen shot 2017-01-18 at 10 11 56

Reset (why doesn't it look like image 1?!) screen shot 2017-01-18 at 10 12 23

I also get a 422 from ETEngine.

""heating_households" group does not balance: group sums to 200.00000000000006 
using 
households_space_heater_coal_share=10.0 
households_space_heater_combined_network_gas_share=10.000000000000027 
households_space_heater_crude_oil_share=20.0 
households_space_heater_district_heating_steam_hot_water_share=10.0 
households_space_heater_electricity_share=5.0 
households_space_heater_heatpump_air_water_electricity_share=5.0 
households_space_heater_heatpump_ground_water_electricity_share=5.0 
households_space_heater_hybrid_heatpump_air_water_electricity_share=5.0 
households_space_heater_micro_chp_network_gas_share=5.0 
households_space_heater_network_gas_share=5.0 
households_space_heater_wood_pellets_share=20.0 
test_households_space_heater_coal_share=10.0 
test_households_space_heater_combined_network_gas_share=10.000000000000027 
test_households_space_heater_crude_oil_share=20.0 
test_households_space_heater_district_heating_steam_hot_water_share=10.0 
test_households_space_heater_electricity_share=5.0 
test_households_space_heater_heatpump_air_water_electricity_share=5.0 
test_households_space_heater_heatpump_ground_water_electricity_share=5.0 
test_households_space_heater_hybrid_heatpump_air_water_electricity_share=5.0 
test_households_space_heater_micro_chp_network_gas_share=5.0 
test_households_space_heater_network_gas_share=5.0 
test_households_space_heater_wood_pellets_share=20.0"
ploh commented 7 years ago

Reset (why doesn't it look like image 1?!)

OK, thx for testing. I will investigate when I am this far in the migration

antw commented 7 years ago

I also get a 422 from ETEngine.

"heating_households" group does not balance: group sums to 200.00000000000006 
using 
households_space_heater_coal_share=10.0 
households_space_heater_combined_network_gas_share=10.000000000000027 
households_space_heater_crude_oil_share=20.0 
households_space_heater_district_heating_steam_hot_water_share=10.0 
households_space_heater_electricity_share=5.0 
households_space_heater_heatpump_air_water_electricity_share=5.0 
households_space_heater_heatpump_ground_water_electricity_share=5.0 
households_space_heater_hybrid_heatpump_air_water_electricity_share=5.0 
households_space_heater_micro_chp_network_gas_share=5.0 
households_space_heater_network_gas_share=5.0 
households_space_heater_wood_pellets_share=20.0 
test_households_space_heater_coal_share=10.0 
test_households_space_heater_combined_network_gas_share=10.000000000000027 
test_households_space_heater_crude_oil_share=20.0 
test_households_space_heater_district_heating_steam_hot_water_share=10.0 
test_households_space_heater_electricity_share=5.0 
test_households_space_heater_heatpump_air_water_electricity_share=5.0 
test_households_space_heater_heatpump_ground_water_electricity_share=5.0 
test_households_space_heater_hybrid_heatpump_air_water_electricity_share=5.0 
test_households_space_heater_micro_chp_network_gas_share=5.0 
test_households_space_heater_network_gas_share=5.0 
test_households_space_heater_wood_pellets_share=20.0

This looks like you have a whole second set of inputs (test_*) assigned to the "heating_households" group? I assume these are inputs you have created, but what is their purpose?

AlexanderWirtz commented 7 years ago

This looks like you have a whole second set of inputs (test_*) assigned to the "heating_households" group? I assume these are inputs you have created, but what is their purpose?

Indeed, @grdw states above that the initializer inputs were added to the heating_households share group.

grdw commented 7 years ago

I'm guessing that the fix is to just use a different share_group. The only thing I changed was prefixing the share_group with test_ so it becomes: test_heating_households. That fixes it but I don't agree with it being a nice way to solve this problem. How come that these values even enter the database? They should be initializer inputs and should (in my mind) therefor only live inside of the .ad file and not magically appear in the balanced_values column. But that's just my 2 cents.

ploh commented 7 years ago

How come that these values even enter the database?

If they have the same share_group as other inputs, then they will be written to the database together with these - because they are balanced together.

Does this still happen when you assign them their own share_group?

Also, I think it would be even better to have no share_group at all for init. inputs.

antw commented 7 years ago

Indeed, @grdw states above that the initializer inputs were added to the heating_households share group.

Thanks, I missed that. :blush:

That does explain why they are appearing. @ploh is therefore correct that Balancer will be including them when trying to auto-balance the group.

With this said, I thought initializer inputs were to be an entirely separate class from Input (see also here); was that idea quietly dropped? :confused: It would certainly avoid this sort of problem...

ploh commented 7 years ago

With this said, I thought initializer inputs were to be an entirely separate class from Input (see also here); was that idea quietly dropped? :confused: It would certainly avoid this sort of problem...

Yes, that was the plan. We kind of dropped it for the moment - quietly indeed, very sorry about that! - in order for the migration to be easier. But... I am now thinking that the migration might even be easier if we invented a new class and then auto-created documents for that on-the-fly during migration where necessary.

antw commented 7 years ago

Yes, that was the plan. We kind of dropped it for the moment

Several issues suddenly make a lot more sense to me. 😆

I am now thinking that the migration might even be easier if we invented a new class and then auto-created documents for that on-the-fly during migration where necessary.

Personally, I do think keeping Input and InitializerInput separate will avoid the potential for pain later (no risk of assigning to share groups, creating duplicate input names, etc). On the other hand, if there is a lot of cross-over between normal inputs and init. inputs I can see why you'd go in the direction you have.

A compromise might be to allow initializer inputs to define either a custom query, or to delegate to an existing input. That would keep them explicit while avoiding unnecessary duplication.

etsource/initializer_inputs/custom_query.ad
- query = UPDATE(V(thing), attribute, USER_INPUT())
etsource/initializer_inputs/delegate.ad
- delegate_to = households_number_of_inhabitants

Now I'm really off-topic; sorry. :blush: