gh-138122: Replace --interval with --sampling-rate #143085
Open
+141
−99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sampling rate is more intuitive to the number of samples per second taken, rather than the intervals between samples.
pablogsal
reviewed
Dec 23, 2025
| * - Default for ``--interval`` / ``-i`` | ||
| - 100 µs between samples (~10,000 samples/sec) | ||
| * - Default for ``--sampling-rate`` / ``-r`` | ||
| - 10 kHz |
Member
There was a problem hiding this comment.
Please update the documentation to reflect the actual 1 kHz default
pablogsal
reviewed
Dec 23, 2025
| help="sampling interval", | ||
| "-r", | ||
| "--sampling-rate", | ||
| type=_parse_sampling_rate, |
Member
There was a problem hiding this comment.
Nit: The argument is named sampling_rate but after parsing it stores the interval in microseconds, not the rate in Hz. This is kind of confusing in the rest of the code
pablogsal
reviewed
Dec 23, 2025
|
|
||
| match = _RATE_PATTERN.match(rate_str) | ||
| if not match: | ||
| raise argparse.ArgumentTypeError( |
Member
There was a problem hiding this comment.
Nit: Let's add a hint about spaces in the error message since "10 khz" (with space) is rejected but users might try it
pablogsal
reviewed
Dec 23, 2025
| return process | ||
|
|
||
|
|
||
| _RATE_PATTERN = re.compile(r'^(\d+(?:\.\d+)?)(hz|khz|k)?$', re.IGNORECASE) |
Member
There was a problem hiding this comment.
Suggested change
| _RATE_PATTERN = re.compile(r'^(\d+(?:\.\d+)?)(hz|khz|k)?$', re.IGNORECASE) | |
| _RATE_PATTERN = re.compile(r''' | |
| ^ # Start of string | |
| ( # Group 1: The numeric value | |
| \d+ # One or more digits (integer part) | |
| (?:\.\d+)? # Optional: decimal point followed by digits | |
| ) # Examples: "10", "0.5", "100.25" | |
| ( # Group 2: Optional unit suffix | |
| hz # "hz" - hertz | |
| | khz # "khz" - kilohertz | |
| | k # "k" - shorthand for kilohertz | |
| )? # Suffix is optional (bare number = Hz) | |
| $ # End of string | |
| ''', re.VERBOSE | re.IGNORECASE) |
Member
There was a problem hiding this comment.
Haha you know how much I like to comment these :)
pablogsal
reviewed
Dec 23, 2025
Member
pablogsal
left a comment
pablogsal
left a comment
There was a problem hiding this comment.
LGTM once the documentation is updated! 🚀
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Towards #142927.
CC: @pablogsal @ivonastojanovic