Skip to content

Conversation

sergeymatov
Copy link
Contributor

No description provided.

@sergeymatov sergeymatov requested a review from a team as a code owner August 21, 2025 10:14
@sergeymatov sergeymatov requested review from Fredi-raspall and removed request for a team August 21, 2025 10:14
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch from 767001e to fd77a5d Compare August 21, 2025 10:33
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch from fd77a5d to e4dcaef Compare August 21, 2025 19:33
@sergeymatov sergeymatov marked this pull request as draft August 22, 2025 09:29
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch 2 times, most recently from 8221155 to d64e269 Compare August 22, 2025 11:10
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch 20 times, most recently from 6b7b586 to 5104932 Compare August 26, 2025 09:10
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch from b98bcdb to 1ef6d8e Compare August 26, 2025 15:39
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch from 1ef6d8e to eb16a28 Compare August 26, 2025 15:40
@sergeymatov sergeymatov marked this pull request as ready for review August 26, 2025 15:45
@mvachhar mvachhar added the ci:+vlab Enable VLAB tests label Aug 26, 2025
@mvachhar mvachhar closed this Aug 26, 2025
@mvachhar mvachhar reopened this Aug 26, 2025
@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch 2 times, most recently from d02c903 to f16a6db Compare August 28, 2025 12:25
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the full context here, and I didn't review in-depth - but assuming the PR is ready for review, it seems to me like this misses unit tests to validate the processing, in particular to validate the Smooth implementation.

@sergeymatov sergeymatov force-pushed the pr/smatov/calc-rates branch 6 times, most recently from 6e4ec54 to 6c4ffec Compare September 2, 2025 11:49
@sergeymatov sergeymatov requested a review from qmonnet September 2, 2025 11:54
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, although I haven't been as far as checking the smoothing algo. Thanks for adding the unit tests!

I've got some minor nitpicks, they should all be trivial to address.

I'd welcome a bit more of context in the commit descriptions - I don't like empty commit descriptions. In something like “No more double-counting for packets”, for example, I'd love to find an explanation of the problem we solve: how comes that we had double-counting, and how do we solve it? Or in “smoothing instead of derivative”, it would be nice to mention why we want to switch from the latter to the former. It is more work, but it makes life so much easier when someone looks at the Git history later to try to understand a change.

}
}

// Push the cumulative snapshot into the SG derivative filter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's SG, I suppose it's “Savitzky-Golay”? Please expand the first time you use it in the comments.

Suggested change
// Push the cumulative snapshot into the SG derivative filter
// Push the cumulative snapshot into the Savitzky-Golay (SG) derivative filter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, this comment is not exists in latest revision of the file :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, it doesn't matter, I'd still like you to expand the acronym the first time you use it in the file, whether it's this comment or another. There definitely are occurrences of this acronym in your latest revision.

@sergeymatov
Copy link
Contributor Author

@qmonnet thanks for the review. In general we are saving Savitsky-Golay derivative calculation cause in particular it's should give same results using different approximation formula. The reason we picked smoothing instead - during tests it showed more stable results, so there is no algorithm decisions was made but based on practical usage.

@sergeymatov sergeymatov requested a review from qmonnet September 3, 2025 10:26
@qmonnet
Copy link
Member

qmonnet commented Sep 3, 2025

Thanks for the context ❤️. Don't hesitate to add it to the commit descriptions next time, this is where I search for it first when I want to understand a code change.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like the SG acronym to be expanded once, but this is not a blocking issue. PR seems good from my side otherwise. Thanks!

@sergeymatov sergeymatov added this pull request to the merge queue Sep 3, 2025
Merged via the queue into main with commit a9312dd Sep 3, 2025
19 checks passed
@sergeymatov sergeymatov deleted the pr/smatov/calc-rates branch September 3, 2025 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:+vlab Enable VLAB tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants