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

♻️ change nan default handling behavior to SkipNa #28

Merged
merged 5 commits into from
Feb 25, 2023

Conversation

jvdd
Copy link
Owner

@jvdd jvdd commented Feb 25, 2023

Change default behavior for nan-handling (i.e., argminmax) to ignore (or skip) nans instead of returning (the first) nan index

Reasons why argminmax default will ignore nans:
✔️ no performance regressions for default implementation
✔️ backwards compatible
🤝 same as default pandas & polars behavior
↔️ numpy default (which returns nan index)

Other contributions of this PR:

  • updated benchmarks (use original names & make these default to ignore-nan variant) => should result in 0 regressions
    • remove unused benchmark functions for float benchmarks
  • renamed the float_simd.rs to more concrete names w.r.t. their nan handling behavior

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 25, 2023

CodSpeed Performance Report

Merging #28 nans_change_default (439b58b) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 32 untouched benchmarks

🆕 20 new benchmarks
⁉️ 12 dropped benchmarks

Benchmarks breakdown

Benchmark nans_v3 nans_change_default Change
🆕 scalar_random_long_f16 N/A 3.3 ms N/A
🆕 sse_random_long_f16 N/A 476.5 µs N/A
🆕 avx2_random_long_f16 N/A 236.4 µs N/A
🆕 impl_random_long_f16 N/A 236.6 µs N/A
🆕 scalar_nanargminmax_f32 N/A 2.2 ms N/A
🆕 sse_nanargminmax_f32 N/A 968 µs N/A
🆕 avx2_nanargminmax_f32 N/A 466.9 µs N/A
🆕 impl_nanargminmax_f32 N/A 467.1 µs N/A
🆕 scalar_random_long_f32 N/A 1.7 ms N/A
🆕 sse_random_long_f32 N/A 712.1 µs N/A
🆕 avx_random_long_f32 N/A 403 µs N/A
🆕 impl_random_long_f32 N/A 403.2 µs N/A
🆕 scalar_nanargminmax_f64 N/A 2.4 ms N/A
🆕 sse_nanargminmax_f64 N/A 2.3 ms N/A
🆕 avx2_nanargminmax_f64 N/A 1.1 ms N/A
🆕 impl_nanargminmax_f64 N/A 1.1 ms N/A
🆕 scalar_random_long_f64 N/A 1.9 ms N/A
🆕 sse_random_long_f64 N/A 1.4 ms N/A
🆕 avx_random_long_f64 N/A 804.7 µs N/A
🆕 impl_random_long_f64 N/A 804.9 µs N/A
⁉️ scalar_random_long_f32 1.7 ms N/A N/A
⁉️ sse_random_long_f32 712 µs N/A N/A
⁉️ avx_random_long_f32 402.9 µs N/A N/A
⁉️ impl_random_long_f32 403.1 µs N/A N/A
⁉️ scalar_random_long_f64 1.9 ms N/A N/A
⁉️ sse_random_long_f64 1.4 ms N/A N/A
⁉️ avx_random_long_f64 804.5 µs N/A N/A
⁉️ impl_random_long_f64 804.7 µs N/A N/A
⁉️ scalar_random_long_f16 2.3 ms N/A N/A
⁉️ sse_random_long_f16 475.6 µs N/A N/A
⁉️ avx2_random_long_f16 235.6 µs N/A N/A
⁉️ impl_random_long_f16 235.9 µs N/A N/A

@jvdd
Copy link
Owner Author

jvdd commented Feb 25, 2023

image

⬆️ only regression is for scalar f16 implementation (highlighted in red) - but this regression was already present in the nans_v3 branch (due to the check for nan to return when first nan encountered)

@jvdd jvdd merged commit 049e9d3 into nans_v3 Feb 25, 2023
@jvdd jvdd mentioned this pull request Feb 25, 2023
23 tasks
@jvdd jvdd deleted the nans_change_default branch February 28, 2023 21:16
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.

1 participant