search

AICoE / prometheus-anomaly-detector

A newer more updated version of the prometheus anomaly detector (https://github.com/AICoE/prometheus-anomaly-detector-legacy)
GNU General Public License v3.0
597 stars 151 forks source link

Adding LSTM model #123

Closed pallekc91 closed 4 years ago

review-notebook-app[bot] commented 4 years ago

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

ghost commented 4 years ago

Build failed.

4n4nd commented 4 years ago

Some formatting issues that the coala bot found:


model_lstm.py
| 105| ············prediction·=·model.predict(data_test.reshape(1,1,self.number_of_features)).flatten()[0]
|    | [NORMAL] PycodestyleBear (E231):
|    | E231 missing whitespace after ','

model_lstm.py
| 107| ············scaled_final_value·=·self.scalar.inverse_transform(curr_pred_value.reshape(1,-1)).flatten()[0]
|    | [NORMAL] PycodestyleBear (E231):
|    | E231 missing whitespace after ','

model_lstm.py
| 119| ························+·(np.std(forecast_values[:i])·*·2)
|    | [NORMAL] PycodestyleBear (E131):
|    | E131 continuation line unaligned for hanging indent

model_lstm.py
| 131| ························-·(np.std(forecast_values[:i])·*·2)
|    | [NORMAL] PycodestyleBear (E131):
|    | E131 continuation line unaligned for hanging indent

model_lstm.py
|   1| import·logging
|    | [NORMAL] PyDocStyleBear:
|    | D100: Missing docstring in public module

model_lstm.py
|  34| ····def·prepare_data(self,·data):
|    | [NORMAL] PyDocStyleBear:
|    | D102: Missing docstring in public method

model_lstm.py
|  50| ····def·get_model(self,·lstm_cell_count,·dense_cell_count):
|    | [NORMAL] PyDocStyleBear:
|    | D102: Missing docstring in public method

model_lstm.py
|  58| ····def·train(self,·metric_data=None,·prediction_duration=15):
|    | [NORMAL] PyDocStyleBear:
|    | D102: Missing docstring in public method
``
ghost commented 4 years ago

Build failed.

4n4nd commented 4 years ago

@pallekc91 A couple more issues


model_lstm.py
|  35| ····def·prepare_data(self,·data):
|    | [NORMAL] PyDocStyleBear:
|    | D400: First line should end with a period (not 'M')

model_lstm.py
|  52| ····def·get_model(self,·lstm_cell_count,·dense_cell_count):
|    | [NORMAL] PyDocStyleBear:
|    | D400: First line should end with a period (not 'l')

model_lstm.py
|  61| ····def·train(self,·metric_data=None,·prediction_duration=15):
|    | [NORMAL] PyDocStyleBear:
|    | D400: First line should end with a period (not 'l')```
MichaelClifford commented 4 years ago

@pallekc91 @usef-kh, looks like there are only a couple minor formatting issues in model_lstm.py. Think you could make those edits and push the updates?

@4n4nd not sure why the thoth is failing. It has something to do with fbprophet do you know what they need to do to pass that check?

pallekc91 commented 4 years ago

Hi Michael,

I fixed the formatting issues and checked in the code. I was waiting on finding out if there is a way to find out all the formatting issues and fix them together. Hence the delay.

Please let me know if there are any such ways. Would be able to fix it once in for all.

Thanks again.

Best, Krishna Palle.

On Wed, 10 Jun, 2020, 2:40 pm Michael Clifford, notifications@github.com wrote:

@pallekc91 https://github.com/pallekc91 @usef-kh https://github.com/usef-kh, looks like there are only a couple minor formatting issues in model_lstm.py. Think you could make those edits and push the updates?

@4n4nd https://github.com/4n4nd not sure why the thoth is failing. It has something to do with fbprophet do you know what they need to do to pass that check?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AICoE/prometheus-anomaly-detector/pull/123#issuecomment-642188890, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANEYMQHVFMDE7B2FIWS36CLRV7HR5ANCNFSM4NA6ZHYA .

4n4nd commented 4 years ago

/retest

4n4nd commented 4 years ago

@4n4nd not sure why the thoth is failing. It has something to do with fbprophet do you know what they need to do to pass that check?

@MichaelClifford where do you see the issues with fbprophet? I cannot find it

harshad16 commented 4 years ago

recheck

ghost commented 4 years ago

Build succeeded.

harshad16 commented 4 years ago

recheck

ghost commented 4 years ago

Build succeeded.

4n4nd commented 4 years ago

@MichaelClifford Could you please review the notebooks?

@pallekc91 @usef-kh I was able to test the model_lstm.py and it works :+1: The only issue I can see is that the model is being created from scratch for every run. Could you please try to pickle the created model and load it for every new run? I think we discussed about this in one of the meetings. Create a new PR for model pickling, once the notebooks are reviewed we can merge this. yay!

usef-kh commented 4 years ago

Hello,

Yes we are aware of that and remember discussing this with you throughout one of our meetings. We’ll look inti this and get back to you as soon as possible.

Best, Yousif

On Wed, Jun 10, 2020 at 11:40 PM Anand Sanmukhani notifications@github.com wrote:

@MichaelClifford https://github.com/MichaelClifford Could you please review the notebooks?

@pallekc91 https://github.com/pallekc91 @usef-kh https://github.com/usef-kh I was able to test the model_lstm.py and it works 👍 The only issue I can see is that the model is being created from scratch for every run. Could you please try to pickle the created model and load it for every new run? I think we discussed about this in one of the meetings. Create a new PR for model pickling, once the notebooks are reviewed we can merge this. yay!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AICoE/prometheus-anomaly-detector/pull/123#issuecomment-642387030, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK76GKOOFK67W4BVZEIADLDRWBG4HANCNFSM4NA6ZHYA .

MichaelClifford commented 4 years ago

Correct me if I'm wrong, but I think the notebooks aren't part of the application at all and are just here for explanation,right?

Also what is the difference between notebooks/prometheus_anomaly_detection_lstm_test_ipynb.ipynb and notebooks/prometheus_anomaly_detection_finallstm_test_ipynb.ipynb ? It looks to me like one is just the updated version of the other. if that's the case can you remove the least up-to-date notebook, and only include 1 in this PR?

usef-kh commented 4 years ago

Hello,

Yes they are purely for explaining.

They tackle the problem in slightly different ways thats why we decided to keep both.

We ended up deploying the FinalLSTM one, but left the other one incase you wanted to have a look at it.

Ofcourse, we could remove it if you’d like.

Best, Yousif

On Thu, Jun 11, 2020 at 5:33 PM Michael Clifford notifications@github.com wrote:

Correct me if I'm wrong, but I think the notebooks aren't part of the application at all and are just here for explanation,right?

Also what is the difference between notebooks/prometheus_anomaly_detection_lstm_test_ipynb.ipynb and notebooks/prometheus_anomaly_detection_finallstm_test_ipynb.ipynb ? It looks to me like one is just the updated version of the other. if that's the case can you remove the least up-to-date notebook, and only include 1 in this PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AICoE/prometheus-anomaly-detector/pull/123#issuecomment-642938582, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK76GKPFTFDLK5EID2KEML3RWFESLANCNFSM4NA6ZHYA .

MichaelClifford commented 4 years ago

If they are different, then that's fine to keep them both. :partying_face:

ghost commented 4 years ago

Build succeeded (gate pipeline).