VR-3974: don't upload artifact in log_training_data()#576
Conversation
| 'count': [5, 10, 15]}}, 'type': 'discrete'}, | ||
| 'continuous col': {'histogram': {'float': { | ||
| 'bucket_limits': [0, 2.9, 5.8, 8.7, 11.6, 14.5, 17.4, 20.3, 23.2, 26.099999999999998, 29], | ||
| 'count': [3, 3, 3, 3, 3, 3, 3, 3, 3, 2]}}, 'type': 'float'}, |
There was a problem hiding this comment.
2 is really telling me that this code is wrong, since there were 30 values sent in. Don't just write tests to pass the code you wrote. Write tests to implement the functionality you want. If the code doesn't pass the tests, then it has a bug.
There was a problem hiding this comment.
Ugh I hadn't even noticed the 2. That's super careless of me. I've figured out better tests, now.
| # data within buckets | ||
| assert all(buckets[0] <= series) | ||
| assert all(series <= buckets[-1]) | ||
|
|
||
| # counts correct | ||
| bin_windows = list(zip(buckets[:-1], buckets[1:])) | ||
| for i, (l, r) in enumerate(bin_windows[:-1]): | ||
| assert sum((l <= series) & (series < r)) == counts[i] | ||
| assert sum(buckets[-2] <= series) == counts[-1] |
There was a problem hiding this comment.
This test will pass if buckets[-1] is always 1e9 as long as all data is smaller than that. Same for buckets[0]. Let's assert we found the right boundaries.
There was a problem hiding this comment.
I've added checks for the leftmost and rightmost boundaries.
Would it make sense to check the other buckets as well? I would just be copy&pasting the logic from the util, which I think defeats the purpose of testing functionality.
There was a problem hiding this comment.
No need to check the other ones. Just the cases that are easy to get wrong.
On a side note, you can always verify the intent of the functionality instead of copying the code. For example in this case, we could check 1) are the bucket sizes about the same? 2) do we get the number of buckets we expected? These two are the properties we want. The code in utils is just a way to get to it.
conradoverta
left a comment
conradoverta
left a comment
There was a problem hiding this comment.
Clicked the wrong button.
2 participants
No description provided.