Validate minimum samples for slope calculation - #53
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
===========================================
- Coverage 100.00% 99.20% -0.80%
===========================================
Files 7 7
Lines 251 253 +2
===========================================
Hits 251 251
- Misses 0 1 +1
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if len(packets) < 2 { | ||
| return 0, false | ||
| } | ||
|
|
There was a problem hiding this comment.
Does this actually do anything?
denominator should be 0 for len < 2 because (x - avgX) is 0 for len 1, and for len zero we don't run the calculation loop at all
I'm all for adding an early return to make the code explicit but does it actually fix anything?
There was a problem hiding this comment.
The early return doesn't fix a functional issue, but it makes the minimum input requirement explicit and avoids unnecessary calculations.
There was a problem hiding this comment.
Can we amend the commit message then? :)
There was a problem hiding this comment.
I was reading the implementation and, while comparing it with the WebRTC implementation (https://webrtc.googlesource.com/src/+/refs/heads/main/modules/congestion_controller/goog_cc/trendline_estimator.cc#58), I added the check mainly for simplicity and clarity.
If this doesn't make sense, feel free to close the PR.
There was a problem hiding this comment.
Can we amend the commit message then? :)
sure
There was a problem hiding this comment.
it's fine to add an early return, i think it will make the code easier to follow, it's just that we use commit messages in releases and "Fix prevent invalid slope calculation" would be misleading for people who read releases :)
be8c36c to
c7368e5
Compare
arjunshajitech
changed the title
2 participants
Description
This PR adds a small guard in fitSlope to handle cases where fewer than two packets are available. This avoids calculating a slope with insufficient data and returns safely instead.