Code Review#2
Conversation
emerisly
left a comment
emerisly
left a comment
There was a problem hiding this comment.
The AR function's name is a bit abstract. Maybe a brief comment explaining what AR means and what AR do?
emerisly
left a comment
emerisly
left a comment
There was a problem hiding this comment.
Not sure if I'm missing the spec file. It would definitely help if there's a brief comment specifying what each file's goal is (or spec of each function) in the same file.
emerisly
left a comment
emerisly
left a comment
There was a problem hiding this comment.
The README needs MoRe fancy descriptionssss
emerisly
left a comment
emerisly
left a comment
There was a problem hiding this comment.
Don't know what's going on but ✨ looking good ✨
| def MA(q, res): | ||
| ''' Moving Average''' | ||
|
|
||
| for i in range(1, q+1): |
There was a problem hiding this comment.
Maybe add a comment for this smaller section of code to explain what's happening
| return best_p_bic, best_bic | ||
|
|
||
|
|
||
| def opt_q(df_testing, low_q, up_q, str): |
There was a problem hiding this comment.
Add some comments throughout this method
| assert (str == 'AIC' or str == 'BIC' or str == 'RMSE') | ||
| best_p_aic, best_p_bic, best_p_rmse = 0, 0, 0 | ||
| best_rmse, best_aic, best_bic = float("inf"), float("inf"), float("inf") | ||
| for p in range(low_p, up_p): |
… two changing normal distributions
ReallyNeil
left a comment
ReallyNeil
left a comment
There was a problem hiding this comment.
Ngl this is super clean overall, probably the best codebase health I've seen on CDS. Keep it up kings and queens.
| @@ -0,0 +1,198 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
Overall great code quality here, the only comments I have are on documentation which is a great sign. I think commenting what each method is supposed to do would be the main value, especially if this project ends up continuing on and being taken on by new members. This is a very experience team overall, but I think documenting could help if newer members end up joining the team
| @@ -1 +1,28 @@ | |||
| # Stock-Densities | |||
|
|
|||
There was a problem hiding this comment.
Super sick idea to have OKRs in readme, def stealing this for future projects :D
| @@ -0,0 +1,26 @@ | |||
| from audioop import mul | |||
There was a problem hiding this comment.
This is not standard practice so I cant blame yall for not doing it, but I would love some documentation at top explaining the class purpose, even if it is not super complicated
| return best_q_bic, best_bic | ||
|
|
||
|
|
||
| def arima(low_p, high_p, low_q, high_q, df, key, opt_method): |
There was a problem hiding this comment.
Consider overall documenting the meaning of each parameter as well! The specs are super concise and awesome with this exception. Would probs help lots with other people who would want to use this code
| @@ -0,0 +1,1771 @@ | |||
| { | |||
| "cells": [ | |||
There was a problem hiding this comment.
Lol had to open this file elsewhere: I don't know exactly what y'all are doing but maybe more descriptive variable names + some cell comments would be great
| print(label+' : '+str(value)) | ||
|
|
||
| if result[1] <= 0.05: | ||
| print("strong evidence against the null hypothesis, reject the null hypothesis. Data has no unit root and is stationary") |
|
Awesome stuff! I like starting to add more modularity with the arima py file instead of keeping everything in a jupyter notebook |
8 participants
No description provided.