Closed bohanjason closed 4 years ago
Thanks @bohanjason! What is the error of clean_metric_data() and why we fix it in this way?
The index will get out of range. e.g., the matrix has 10 columns but the index is 20. it will get errors when inserting
The index will get out of range. e.g., the matrix has 10 columns but the index is 20. it will get errors when inserting
I see. You are right. However, we need to make sure the metrics in the matric is always in the same order. Can we sort the missed columns by the index, and insert them from the smallest to the largest?
It looks good to me since we have already sorted it at line 156 : missing_columns = sorted(set(metric_cat) - set(metric_labels)) We then append the missing column in the same order.
You are right. It was written by me but I forgot that part. At that time, I also encountered the index out of range error, so I sorted those indexes and I fixed the bug when the metrics in the middle were missing. Now I think the bug still exists if the last few metrics are missing. Your fix can work in that case, but if each time there are different missing columns then the matrix is not consistent. I do not know if you can check if the metric index is larger than the matrix size, and only append metric in that case, and insert metric in other cases. Thank you for your help again.
Thanks for your comment @yangdsh ! I update the PR, please let me know what you think. This bug needs to be fixed to pass integration tests.
LGTM @dvanaken Thanks! @bohanjason
Awesome thank you both!
fix ddpg when it has missing metrics . add integration tests for ddpg
271