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

add math min and math max to bench command #12913

Merged
merged 1 commit into from
May 20, 2024

Conversation

fdncred
Copy link
Collaborator

@fdncred fdncred commented May 20, 2024

Description

This PR adds min and max to the bench command.

 use std bench
 bench { dply -c 'parquet("./data.parquet") | group_by(year) | summarize(count = n(), sum = sum(geo_count)) | show()' | complete | null } --rounds 100 --verbose
100 / 100
╭───────┬───────────────────╮
 mean   71ms 358µs 850ns  
 min    66ms 457µs 583ns  
 max    120ms 338µs 167ns 
 std    6ms 553µs 949ns   
 times  [list 100 items]  
╰───────┴───────────────────╯

User-Facing Changes

Tests + Formatting

After Submitting

@maxim-uvarov
Copy link
Contributor

I'm not sure why into int | into float is there. Maybe it is historical artifact that isn't necessary animore

timeit { do $code } | into int | into float

Without it we can get rid of from ns

mean: ($times | math avg | from ns)

@amtoine
Copy link
Member

amtoine commented May 20, 2024

@maxim-uvarov
into int is to convert the duration given by timeit to the corresponding number of nanoseconds and into float is here to help the math stddev not overflow 😮

for instance

seq 1 100 | each { random int } | math stddev
Error: nu::shell::operator_overflow

  × Operator overflow.
   ╭─[entry #2:1:13]
 1 │ seq 1 100 | each { random int } | math stddev
   ·             ──┬─
   ·               ╰── multiply operation overflowed
   ╰────
  help: Consider using floating point values for increased range by promoting operand with 'into float'. Note: float has reduced precision!

vs

seq 1 100 | each { random int } | into float | math stddev
2579795210688796000

@maxim-uvarov
Copy link
Contributor

maxim-uvarov commented May 20, 2024

@amtoine got it, thanks

@fdncred
Copy link
Collaborator Author

fdncred commented May 20, 2024

@amtoine are you good with these additions? I think min and max are pretty helpful here.

@amtoine
Copy link
Member

amtoine commented May 20, 2024

@amtoine are you good with these additions? I think min and max are pretty helpful here.

yup, it's fine because mean and std are already there 👍

to have my full opinion, i think i would have only selected times if i were to write this command again nowadays, letting the user compute statistics at will 🤣
because, this begs the question: why not add the math median, math variance or even compute the MAD indicator? 😉

@fdncred
Copy link
Collaborator Author

fdncred commented May 20, 2024

true. let's move ahead with this. it's just software, we can change it if we want later.

@fdncred fdncred merged commit 4f69ba1 into nushell:main May 20, 2024
15 checks passed
@fdncred fdncred deleted the add_min_max_to_bench branch May 20, 2024 15:08
@hustcer hustcer added this to the v0.94.0 milestone May 20, 2024
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.

None yet

4 participants