Skip to content

Commit experiment result tables and fix bug in experiment helper #872

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 14, 2025
Merged

Commit experiment result tables and fix bug in experiment helper #872

merged 3 commits into from
Nov 14, 2025

Conversation

@AbdulRahmanAlHamali
Copy link
Contributor

@AbdulRahmanAlHamali AbdulRahmanAlHamali commented Nov 13, 2025 •
edited
Loading

This PR does two things:

  1. Write tabular results of experiments, and commit those as well. They contain additional data that is sometimes very hard to see in the graphs
  2. There was a bug in the experiment helper that basically caused the processing threads to keep running even when no requests have been added by the arrival thread. This PR fixes that as well, and as a result you will see a change in the graphs where we're currently rejecting much less

@AbdulRahmanAlHamali AbdulRahmanAlHamali changed the base branch from main to pid-take-2 November 13, 2025 21:56
@AbdulRahmanAlHamali AbdulRahmanAlHamali changed the title Commit result tables Commit experiment result tables and fix bug in experiment helper Nov 13, 2025
@AbdulRahmanAlHamali AbdulRahmanAlHamali marked this pull request as ready for review November 13, 2025 22:20
adriangudas
adriangudas reviewed Nov 14, 2025
View reviewed changes
experiments/experiment_helpers.rb
puts "-" * 150
@table_outputs << "=== PID Controller State Per Second (Aggregated across threads) ==="

header = format("%-8s %-10s %-22s %-15s %-22s %-25s %-25s %-15s", "Window", "# Threads", "Err % (min-max)", "Ideal Err %", "Reject % (min-max)", "Integral (min-max)", "Derivative (min-max)", "Total Req Time")
Copy link

@adriangudas adriangudas Nov 14, 2025

Choose a reason for hiding this comment

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

(nit) I also realized we could probably just use CSV format here - I think it looks fairly reasonable as a github preview, and might be easier to work with.

Copy link
Contributor Author

@AbdulRahmanAlHamali AbdulRahmanAlHamali Nov 14, 2025

Choose a reason for hiding this comment

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

good call. I'll switch to csv format

Aguasvivas22
Aguasvivas22 approved these changes Nov 14, 2025
View reviewed changes
Copy link

@Aguasvivas22 Aguasvivas22 left a comment

Choose a reason for hiding this comment

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

lgtm

@AbdulRahmanAlHamali AbdulRahmanAlHamali merged commit fe69b9f into pid-take-2 Nov 14, 2025
32 checks passed
@AbdulRahmanAlHamali AbdulRahmanAlHamali deleted the commit-result-tables branch November 14, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Aguasvivas22 Aguasvivas22 Aguasvivas22 approved these changes

@adriangudas adriangudas adriangudas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AbdulRahmanAlHamali @Aguasvivas22 @adriangudas