Update calculate_results to calculate statistics excluding trimmed samples #12
Conversation
Modify string literals to contain parsec where 3pc is currently expected. Signed-off-by: Michael Maurer <[email protected]>
Update variable names, comments where "Phase Two" is mentioned to reference PArSEC instead Signed-off-by: Michael Maurer <[email protected]>
Calculation of statistics now omits the first TRIM_SAMPLES samples Signed-off-by: Michael Maurer <[email protected]>
|
N.B.: includes #11 |
|
Concept ACK. However, actually deploying the test controller with this change causes test result calculation (and recalculation with trimming) to exit with a failing status ( Exploring more carefully locally. Edit to add: I actually don't think the failure I'm seeing is coming from this change, because it seems to be happening elsewhere now. Consider this a ut-ACK and a 👍; let's chat later about what could be causing this issue! |
|
I have some good time to merge things today on the TC, so I'm redeploying and testing this now! (N.B.: #17 is purely for testing; this PR will be merged if testing is successful.) |
| for i in range(len(tps_lines)): | ||
| tps_lines[i]["tps"] = tps_lines[i]["tps"][trim_samples:] | ||
| for i in range(len(lat_lines)): | ||
| lat_lines[i]["lats"] = lat_lines[i]["lats"][trim_samples:] |
There was a problem hiding this comment.
@maurermi my python is a bit rusty. I was expecting this code to trim the first trim_samples samples from the set, but that doesn't appear to be what's happening when I go to test (the beginning of the test is preserved, and clearly the last trim_samples samples are removed).
It's pretty clear the previous version of the code did exactly that, but I'm not sure why. Perhaps this should be merged (since it correctly applies the trimming to the stats and charts), and we should consider offering a manual trim-from-begin and trim-from-end inputs? (would mirror the auto-trim-zeros split well)
|
Assuming this comment is expected, t-ACK, and this is ready-to-merge. If not, then we should evaluate next steps. |
|
will need manual-rebase as #11 was merged. |
No description provided.