first implementation of steel model with revised structure #8
Conversation
|
|
||
|
|
||
| gi_values = gi_distribution.values.transpose() | ||
| at_a = np.matmul(gi_values.transpose(), gi_values) |
There was a problem hiding this comment.
Can we have some more self-explanatory variable names here please?
There was a problem hiding this comment.
sure, definitely. I wasn't really sure how though - gi_values I can definitely rename, at_a here means matrix a transposed times matrix a, basically I am solving for a parameter here according to a standard optimization approximation Jakob proposed (to minimize Ax-b). I guess the options are to either to
- squish all the lines into one to avoid using that variable at all
- Come up with some sort of variable name that describes this
- Keep it as it is but make a better explanatory comment before
What do you think would be best? I'm tending towards option 3, but am happy to go with something else (I know it probably doesn't matter too much but wanted to ask cause I don't have a good feel for how to write very self-explanatory code yet).
There was a problem hiding this comment.
Okay, yes it's difficult when there isn't really a physical meaning, and squashing all of that into line 74 is probably too much. I also think a comment is the best option.
| elif fitting_function_type == 'exponential': | ||
| prms_out = least_squares( | ||
| exponential_fitting_function, | ||
| x0=np.array([400, 1]), |
There was a problem hiding this comment.
Is this supposed to be the same x0 as used in the sigmoid case?
There was a problem hiding this comment.
No, I think they should be different, at least the second one (the first one should be the saturation level which I guess should be similar with both approaches, but the way it is calculated in the sigmoid case is I think specific to the plastics model [I guess it might make sense to change that structure at some point as this is in the common section now - e.g. by specifying in the config whether and how the initial guess of saturation level and other parameters should be calculated?]).
In the former steel model, we had different version of this regression, one, where the parameters where optimised like here and one where we calculated it numerically. As I changed the inflow driven model calculation, I steel need to look at the numbers to make this decision so this might change soon again.
There was a problem hiding this comment.
Okay, I didn't release this was a model specific initial guess. If you were using both options (exponential and sigmoid) for the steel model (e.g. to compare results from different approaches), you would then have the same x0, right? Or is it also dependent on the fitting function? Maybe, we could pass x0 as a parameter to the gdp_regression function, but I think I would keep hard-coded values somewhere (and not require the user to specify it in the config), maybe directly in the steel and plastic models, or putting (or calling) this gdp_regression in the steel / plastic directories rather than common. For now, please just add a little comment to the code with your explanation above and I will think what to do with it later!
There was a problem hiding this comment.
The initial guess x0 would be different for the different fitting functions, at least for the second parameter. In both fitting functions here the first parameter describes the saturation level, whilst the second one defines some other valuable that influences the curvature of the function which will be very different depending on the form of the function. One could also imagine fitting functions which don't use a saturation level at all in the future, hence they should always be different. I will add the comment like you said and as said I also need to look at the numbers a little more myself - am happy to change the location of the x0 values with whatever you propose!
| for i_region in range(shape_out[1]): | ||
| for i_good in range(shape_out[2]): |
There was a problem hiding this comment.
Not sure whether this would work, but:
| for i_region in range(shape_out[1]): | |
| for i_good in range(shape_out[2]): | |
| for i_good in range(shape_out[2]): | |
| historic_good = historic_stocks_pc[:, :, i_good] |
If we do the above, could we take out the loop over region and calculate using arrays?
e.g.
def sigmoid_fitting_function(prms):
return prms[0] / (1 + np.exp(prms[1]/gdppc[:n_historic])) - historic_good
There was a problem hiding this comment.
Hmm good point, I actually just adopted this from what was here which I am assuming then stemmed from the plastics model (or my memory is getting quite bad and I coded this some time ago :) ). I can try it out, I guess it could even be possible to take out the loop over goods as well, we might need to reshape the initial parameters then which might look messy, depends a bit on scipy, I'm going to look into it.
There was a problem hiding this comment.
Nice, yes I know this was not actually in your pr, but the whole function is a bit messy, so I was thinking how we can make it a bit cleaner. Yeah, reshaping parameters might be less readable than one loop (or maybe not, I'm not sure). It would likely be more efficient to reshape instead of looping, but I don't think we have an efficiency problem at the moment, so readability would be my priority!
There was a problem hiding this comment.
So I looked into it now and SciPy requires x0 to be maximum one dimensional, meaning that if we canceled one or both loops, we would need to 'flatten' x0 so that (if we just looped over regions) x0 would look something like this [sat_level_good_0, sat_level_good_1, sat_level_good_2, sat_level_good_3, curvature_prm_good_0, curvature_prm_good_1, curvature_prm_good_2, curvature_prm_good_3], which I think would severely effect readability and following up from what you said I think I would leave it as it is for now (unless we find another solution).
delete unnecessary lines Co-authored-by: SallyDa <sallyda@pik-potsdam.de>
delete unnecessary init function Co-authored-by: SallyDa <sallyda@pik-potsdam.de>
Co-authored-by: SallyDa <sallyda@pik-potsdam.de>
Added new inflow driven model and adapted steel model to the new, more clear structure. Still not running completely smoothly (some problem in the use process) but overall a first start.