Review / Feedback - 18 April 2023

[ilfrich]

Some small fixes: 8b1814d

Battery Model

• Should we init min_soc and max_soc with 0 and 100 respectively?
• Should we maybe have a to_json(file_path: Optional[str] = None) -> dict and @staticmethod from_json(json: Union[dict, str]) -> BatteryModel to de-/serialise the battery model config (useful for transmitting it via APIs or storing it in .json files)
• default_battery_params should probably be replaced with a get_default_battery_params() function that returns a copy of the dictionary, so that any local mutation to the dictionary is not reflected in other parts of the application.

Controllers

AbstractBatteryController

• Should we maybe pass the BatteryModel in as constructor argument? Reasoning is: a controller instance should be responsible for one specific battery instance. Can be None for DoNothingController.
• When you run this in production, I think the solve(..) method should have a parameter for the SoC at the beginning of the optimisation period, where you pass in the current exact SoC retrieved from the API connecting to the physical battery. If the parameter is left out (default = None), it will fall back to the BatteryModel's soc attribute (which supports the research/test scenario).
• I would like to put calculate_solution_performance into this class as a method, it can use the battery_model from the controller instance and have a parameter that can override the initial soc. And also remove the param part in the function's signature as well as body (as it is not used for anything)
• We should probably have the debug flag / mechanism for all controller classes and just inherit this from the AbstractBatteryController. This might also make it much easier to provide a ._debug(args**) function that checks the flag and dumps whatever is passed to print(..)
• I don't quite like that we pass all parameters as dictionary to the controller constructor. Especially for controllers, where there's only 1 or 2 parameters, I feel they should be listed explicitly in the constructor for a better programming experience. For lots of params, such as the DP controller, we can still fall back to the params dict.

DynamicProgram

• this_solar_curtailment is used in the code, but the variable is never defined
• Should rename this to DynamicProgramController (to be consistent with the other controllers)

SpotPriceArbitrageOptimal

• Should rename this to SpotPriceArbitrageOptimalController (to be consistent with the other controllers)

Utilities

• For usage of get_discretisation_offset should we maybe determine the precision parameter dynamically, depending on the soc_interval of the controller? So, if the soc_interval is 0.5 a precision of 1 is sufficient, if it's 0.005, then 3 is required (default is 2 for 2 decimal places).
• I think for the package export, we should from oes.util import general **as utility**, ... as general doesn't seem like a good variable to have. Alternatives: oes_util or oesutil

[juliandehoog]

Thank you @ilfrich! Yes to almost everything, except the below:

When you run this in production, I think the solve(..) method should have a parameter for the SoC at the beginning of the optimisation period, where you pass in the current exact SoC retrieved from the API connecting to the physical battery. If the parameter is left out (default = None), it will fall back to the BatteryModel's soc attribute (which supports the research/test scenario).

I want to avoid having battery state of charge defined in two different places.

I wasn’t even sure, to be honest, if BatteryModel.soc is the best place to store that. Maybe the battery model should be just that: a set of parameters that define the model. Whereas an instance of Battery (doesn’t exist yet) could have both a BatteryModel, as well as an actual state defined by whatever variables, such as current state of charge. What do you think? I might be overthinking it and simpler is probably better for now.

I would like to put calculate_solution_performance into this class as a method, it can use the battery_model from the controller instance and have a parameter that can override the initial soc. And also remove the param part in the function's signature as well as body (as it is not used for anything)

I somewhat prefer to keep calculate_solution_performance outside of the controller classes. This is supposed to be an external, "neutral", accurate way to determine how the solution of a controller would actually perform in a particular scenario. Open to discussing this though.

I don't quite like that we pass all parameters as dictionary to the controller constructor. Especially for controllers, where there's only 1 or 2 parameters, I feel they should be listed explicitly in the constructor for a better programming experience. For lots of params, such as the DP controller, we can still fall back to the params dict.

I guess the main motivation for this is that later, when building schedules, it's nice to be able to pass a single set of parameters to all controllers that are used to build the schedule.

But yes to all other changes. I expect to push update to DynamicProgram and SpotPriceArbitrageOptimal soon.

[ilfrich]

Got a bit distracted from this recently. Will resume work on this.

My main thinking behind the calculate_solution_performance change was to put that into a base controller class (abstract), which can be used by any implementing sub-controller (but easily accessible, rather than via another module import). By having it located in the base controller class, it remains independent from individual controller implementations and still has access to the controller data. But not married to the idea, so for now we can potentially keep it in its original place (utility).

The other main problem is how to manage the SOC. I think having 2 classes BatteryModel and Battery would be too convoluted. I agree that there probably should be ONE way to use and pass SOCs. Updating the SOC at each step through controller.battery_model.set_soc(..) or something like that seems too unintuitive. Ultimately, I think the SOC is only relevant for executing an optimisation on a specific state, which means the controller would be the best way to store the SOC. On the other hand you want to be able to investigate prior SOCs at prior optimisation steps. Will look through that code again and see what would work best, but I get your point.

[ilfrich]

Ok, I've implemented new types AbstractBattery(ABC) and SimulatedBattery(AbstractBattery). Each subclass of AbstractBattery needs to implement get_current_soc(). In case of a real battery, this would make a socket/http call to fetch the actual SOC.

• The simulated battery allows to override_soc(new_soc) to manually set the current SOC.
• All controllers now need a AbstractBattery subclass instance passed to their constructor - i.e. there's a 1:1 mapping between battery instances and controllers.
• Access to the battery model goes via battery.model
• Removed any battery or battery_model from the solve() method call (as the controller holds the reference from the constructor)
• all_soc in the AbstractBatteryController now only fetches the initial soc from the battery and then uses the previous SOC to compute the next SOC.

[juliandehoog]

From my point of view, all of the above has been addressed. Your branch was merged successfully. All looks good, all working now.