Logistic Growth Function and Tests #16
Conversation
Update my fork
Update my fork
nsryan2
added
Comp:Core
There was a problem hiding this comment.
Thanks for making this PR! It looks good. I've requested some minor changes and ask that you find another more "pythonic" way to calculate the values for logistic growth (rather than a for-loop).
Finally, don't forget to update the documentation! If it says "TODO: add logistic" and you've done it, ditch that comment!
I'll merge once these changes are made. You can use the "add suggestion to batch" button to reduce the number of emails you send with each change.
pygenesys/utils/growth_model.py
Outdated
| for year in years: | ||
| growth = cap/(1 + np.exp(-growth_rate * (year - sigmoid))) | ||
| growth_data.append(growth) | ||
| np.asarray(growth_data) |
There was a problem hiding this comment.
A few comments
- for loops are slow. can you find a way to do this without using a for loop? (see linear and exponential growth models for examples -- i like lambda functions but pep8 doesn't like them for some reason)
- By using a for-loop, the style of this function is different than the style of the other functions. can you match the styles?
There was a problem hiding this comment.
Also, if you don't introduce growth_data as a list, there is no reason to convert it to an array. The function will be applied to years which is already an np.array.
There was a problem hiding this comment.
Sure thing, I can switch it to match the style of the others!
Co-authored-by: Sam Dotson <[email protected]>
|
@samgdotson Should be all good to go! Thanks for the feedback |
|
@samgdotson, any hesitations on getting this merged or closed? |
This PR adds the capability for a logistic growth model and closes #11.
I based this off of the website in the issue and the wikipedia page.