ProjectDrawdown / solutions

The mission of Project Drawdown is to help the world reach “Drawdown”— the point in the future when levels of greenhouse gases in the atmosphere stop climbing and start to steadily decline, thereby stopping catastrophic climate change — as quickly, safely, and equitably as possible.
https://www.drawdown.org/
Other
211 stars 91 forks source link

Changing advanced controls during building integration #465

Closed danielmk closed 2 years ago

danielmk commented 2 years ago

The logic of building integration (#418) is such that we calculate integrated parameters (efficiency factors and such) and then feed those to the solution before calculating.

I would therefore need to input those integrated parameters into their correct place in AdvancedControls. However, AdvancedControls is a frozen dataclass, therefore I get FrozenInstanceError: cannot assign to field 'soln_energy_efficiency_factor' when I try to assign to an attribute of AdvancedControls.

Could we unfreeze AdvancedControls @denised ?

denised commented 2 years ago

Is this intended to be a temporary or a permanent change?

If it is a permanent change, the approach should be to write out an updated Scenario (a scenario and an AC have a 1:1 relationship; the scenario is basically a code wapper around an AC instance). Then after that, you would request the user to restart their python process, so the new (integration version) scenario would be loaded instead.

If it is a temporary change, this is interesting, but I believe I know how to do it. The reason for having AC (and effectively scenario) be a frozen class is probably because the actual identity (object id) are used as cache keys for lots and lots of computed results. So what you would want to do is: (a) Use the ac.as_dict() procedure to create a plain dictionary with all the AC keys in it. (b) Update the key(s) you need in that dictionary. (c) Use load_scenario with this dict as a 2nd argument to create a brand new scenario instance with the changed AC.

If this is more than a one-off situation, then that 2nd process seems like a good candidate for integration_base.py

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 12:38 AM, Daniel Müller-Komorowska wrote:

The logic of building integration (#418 https://github.com/ProjectDrawdown/solutions/issues/418) is such that we calculate integrated parameters (efficiency factors and such) and then feed those to the solution before calculating.

I would therefore need to input those integrated parameters into their correct place in AdvancedControls. However, AdvancedControls is a frozen dataclass, therefore I get FrozenInstanceError: cannot assign to field 'soln_energy_efficiency_factor' when I try to assign to an attribute of AdvancedControls.

Could we unfreeze AdvancedControls @denised https://github.com/denised ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ProjectDrawdown/solutions/issues/465, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKAMTSIQXWSZADCKVKTRHTUDQTIDANCNFSM5EVK2YOA.

danielmk commented 2 years ago

I intended it as a permanent change and I would have just set @dataclasses.dataclass(eq=True, frozen=False). This works in that it makes attributes changeable but I still get wrong numbers.

I was just wondering whether unfreezing the whole class might be a bad idea.

denised commented 2 years ago

Yes, I think it would be a bad idea. The reason is that there is a lot of stuff in the scenario that has already been calculated based on the value of that AC, and if you just change it in place, you do not trigger a recalculation of all those dependencies.

Creating a new scenario object based on a modified AC input should be the way to go.

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 1:14 AM, Daniel Müller-Komorowska wrote:

I intended it as a permanent change and I would have just set @.***(eq=True, frozen=False)`. This works in that it makes attributes changeable but I still get wrong numbers.

I was just wondering whether unfreezing the whole class might be a bad idea.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ProjectDrawdown/solutions/issues/465#issuecomment-926436234, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKAMTWIDCZBL2VWPRIDY7LUDQXNRANCNFSM5EVK2YOA.

danielmk commented 2 years ago

That's a good point, thanks.

denised commented 2 years ago

...And if this is supposed to be an updated version of that scenario, saved for future use, then we need to write the code that does that. It would either be in Scenario class or AdvancedControls class and would basically dump the AC value in a json file --- but it would use the integration.integration_alt_filename function to determine where to write it. I think I'd put it in Scenario class, since there is other integration-related stuff there.

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 1:17 AM, Denise Draper wrote:

Yes, I think it would be a bad idea. The reason is that there is a lot of stuff in the scenario that has already been calculated based on the value of that AC, and if you just change it in place, you do not trigger a recalculation of all those dependencies.

Creating a new scenario object based on a modified AC input should be the way to go.

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 1:14 AM, Daniel Müller-Komorowska wrote:

I intended it as a permanent change and I would have just set @.***(eq=True, frozen=False)`. This works in that it makes attributes changeable but I still get wrong numbers.

I was just wondering whether unfreezing the whole class might be a bad idea.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ProjectDrawdown/solutions/issues/465#issuecomment-926436234, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKAMTWIDCZBL2VWPRIDY7LUDQXNRANCNFSM5EVK2YOA.

danielmk commented 2 years ago

Right, it would be the integrated version of that scenario. Not sure if I want to worry about saving it at this stage though. I'm not getting correct numbers yet.

denised commented 2 years ago

Fair enough :-)

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 1:28 AM, Daniel Müller-Komorowska wrote:

Right, it would be the integrated version of that scenario. Not sure if I want to worry about saving it at this stage though. I'm not getting correct numbers yet.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ProjectDrawdown/solutions/issues/465#issuecomment-926445357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKAMTVGBWZENPOOSW4TZQLUDQZDVANCNFSM5EVK2YOA.

denised commented 2 years ago

Closed with #471