New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-41807: Add padding to clipDataToEvent #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. I had one minor comment on code de-duplication and another on the unit test.
self.assertGreater(len(clippedPaddedData), 0) | ||
self.assertLess(len(clippedPaddedData), len(dayObsData)) | ||
self.assertGreater(len(clippedPaddedData), len(clippedData)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either instead of or in addition to these assertGreater/assertLess checks, I would really like to see a check that the length of the padded data was the correct amount longer than the unpadded data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I had the same thought. It's actually quite easy to check in this instance, with hard-coded test data, but it's hard in the general case, because a) it's not going be exact, due to the quantized nature of the data, and b) it'll depend on whether or not the input data actually extends out far enough (hence the warning logs for when it doesn't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a swing at that in the morning though, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, pushed a fix for this, it's in Jenkins now.
d8155ce
to
5d2c491
Compare
5d2c491
to
83d85c7
Compare
No description provided.