Skip to content
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.

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

simplify the std bench improvement candidate #865

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

maxim-uvarov
Copy link
Contributor

@amtoine gave me valuable feedback about the PR of std bench improvement CANDIDATE into stdlib-candidates.

I understood and respected his reasoning about his position. Yet, I still believe that users of std bench will benefit from my proposal.

I incorporated some of @amtoine advice in this PR.

I removed the bench --units option as I now believe it is better to encourage users to use core functionality for formatting duration (which I had not thought of initially) and to avoid multiplying --options.

bench -n 10 {sleep 0.1sec} | format duration ms mean min max stddev
# or
bench -n 10 {sleep 0.1sec} | format duration ms ...($in | columns)

Also, I removed the option of --sign-digits and just hard-coded the precision of conversion to the fourth significant digit (which will give a maximum relative error of 0.05%, which I still think is unnecessarily precise).

To explain my motivation, I added some context from our previous conversation:

Maybe you would agree with me that this representation is very wordy and unnecessary, even for a professional?
1sec 333ms 333µs 333ns

maybe, but then i would argue the issue comes from the formatting nushell does on durations, not from std bench, it's just that Nushell will show every part of the duration 😕

And I add here that if Nushell adds the setting for resetting insignificant digits from displaying, those conversions could be removed for the better. Yet, we don't have such a setting yet, but we already use bench and use it quite often. So, I propose this usability improvement.

In my defense, I would add that the existing --pretty option will only benefit from the proposed changes (while it can't benefit from | format duration ms).

> bench {sleep (10sec / 3)} --rounds 2 --pretty
- 3sec 335ms 974µs 264ns +/- 1ms 108µs 748ns
+ 3sec 335ms +/- 1ms 108µs

Finally, I kept the --list-timings option because I strongly believe that users much more often will not need expanded 50 lines of timing results on their screen (which they can get rid of by adding | reject time in the second execution of the bench command - but I would like to avoid this second execution).

I won't be hurt if my proposed changes aren't accepted and applied to mainstream. Yet, I feel like my initial PR is still valuable and will benefit from my current PR's additions.

yet, I hardcoded rounding precision to fourth signinficant digit (which
can give maximum relative error of 0.05%)
@maxim-uvarov maxim-uvarov marked this pull request as ready for review June 3, 2024 13:25
@fdncred fdncred merged commit 308d858 into nushell:main Jun 3, 2024
1 check passed
@fdncred
Copy link
Collaborator

fdncred commented Jun 3, 2024

Thanks

@maxim-uvarov maxim-uvarov deleted the bench-simplif3 branch June 3, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants