Skip to content

ENH: Add SIMD operation copysign - #19770

Closed
howjmay wants to merge 1 commit into
numpy:mainfrom
howjmay:simd-copysign
Closed

ENH: Add SIMD operation copysign#19770
howjmay wants to merge 1 commit into
numpy:mainfrom
howjmay:simd-copysign

Conversation

@howjmay

@howjmay howjmay commented Aug 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

The benchmark run in the CI gave the following response.

    before           after         delta
    [79a8986b]       [bdd62b3]
    <master>         <simd>

-   546±0μs          490±0μs        10.26%  bench_core.PackBits.time_copysign

@howjmay
howjmay force-pushed the simd-copysign branch 15 times, most recently from c466922 to 5bc0d7a Compare August 29, 2021 06:50
@howjmay
howjmay marked this pull request as ready for review August 29, 2021 08:04
@howjmay howjmay changed the title ENH: Add SIM copysign ENH: Add SIMD operation copysign Aug 29, 2021
@seberg seberg added the component: SIMD Issues in SIMD (fast instruction sets) code or machinery label Sep 8, 2021
@rgommers

rgommers commented Sep 11, 2021

Copy link
Copy Markdown
Member

Thanks @howjmay! Would you be able to add the results from an asv benchmark to the PR description? For an example, see gh-17102. benchmarks/bench_ufunc.py seems to have a copysign benchmark; if it doesn't cover the case you are optimizing then maybe you can add a new one?

@howjmay
howjmay force-pushed the simd-copysign branch 4 times, most recently from ae737db to bde468f Compare September 12, 2021 10:39
@howjmay

howjmay commented Oct 4, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @howjmay! Would you be able to add the results from an asv benchmark to the PR description? For an example, see gh-17102. benchmarks/bench_ufunc.py seems to have a copysign benchmark; if it doesn't cover the case you are optimizing then maybe you can add a new one?

Hi @rgommers I have added copysign to the benchmark!

@rgommers

rgommers commented Oct 4, 2021

Copy link
Copy Markdown
Member

Thanks @howjmay! It would be good to add the results to the PR description, in the before after format like in gh-17102. That way it's immediately clear that this PR improves performance and to what extent.

@howjmay

howjmay commented Oct 5, 2021

Copy link
Copy Markdown
Contributor Author

Hi @rgommers, I have appended the result of benchmark ran in CI. Please take a look. Thanks!

rgommers
rgommers reviewed Oct 5, 2021
View reviewed changes

@rgommers rgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @howjmay. The speedup is only ~10% so that seems a bit marginal, but on the other the code complexity here isn't very high. So it may be a decent tradeoff - it's not quite clear to me. Any opinions @seiko2plus, @Qiyu8, @mattip?

Comment thread benchmarks/benchmarks/bench_ufunc.py Outdated
self.f = np.zeros(150000, dtype=np.float32)
self.d = np.zeros(75000, dtype=np.float64)

self.tf = np.ones(150000, dtype=np.int32)

@rgommers rgommers Oct 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this the same as self.i, so you can reuse that instead?

@seberg seberg Oct 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just to point it out, but you do not want to use an integer array in the test. This will trigger casting. You need to use a homogeneous signature (i.e. both inputs are float32 or float64).
The speedup may still not be huge (this is probably memory-bound anyway), but adding a cast might hide most of it even if it is huge.

@howjmay howjmay Oct 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @rgommers, @seberg do you guys know how can I run two benchmark for comparing the original implementation and the SIMD implementation? I have changed the benchmark test for using float64, but the elapsed time increased. I think it is quite weird, and the 10% speedup is weird for my side too. I thought it will accelerate much more.

@rgommers rgommers Oct 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For running the benchmark, you could reorder your commits so the benchmark one comes first, and put a new branch name pointing to that commit. Then you can compare that new branch with the simd-copysign one.

@seberg seberg Oct 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reordering commits should not even be necessary (although maybe nice) if you use --bench-compare from asv through runtests: https://numpy.org/doc/stable/benchmarking.html

Not sure if the docs needs updating, I feel I always have to run it from the benchmarks folder with ../runtests.py.

@seiko2plus seiko2plus Nov 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's a missing understanding here, this patch only adds universal intrinsics for copysign but without actually using them in the inner loop of ufunc so there's no need for a benchmark.

@seiko2plus

seiko2plus commented Nov 3, 2021

Copy link
Copy Markdown
Member

@howjmay, First of all, I would like to thank you for your interest in improving the performance of NumPy, we really appreciate your efforts but again, there's no need to add new intrinsics for copysign for the same reasons I mentioned in #19780 (review).

@howjmay

howjmay commented Nov 16, 2021

Copy link
Copy Markdown
Contributor Author

Thank you for informing!

@howjmay howjmay closed this Nov 16, 2021
@howjmay howjmay reopened this Nov 16, 2021
@InessaPawson

InessaPawson commented Aug 10, 2022

Copy link
Copy Markdown
Member

@howjmay Did you mean to reopen this PR? If not, please close it once again when you get a chance.

@seiko2plus seiko2plus mentioned this pull request Dec 30, 2022
Closed
@charris

charris commented Feb 20, 2023

Copy link
Copy Markdown
Member

@seiko2plus Should this be closes (again)?

@charris charris added the triage review Issue/PR to be discussed at the next triage meeting label Feb 20, 2023
@seiko2plus seiko2plus added the 57 - Close? Issues which may be closable unless discussion continued label Feb 21, 2023
@seiko2plus

seiko2plus commented Feb 21, 2023

Copy link
Copy Markdown
Member

@charris, yes, unless if @howjmay decided to re-implemented similar to #19780

@charris

charris commented Feb 21, 2023

Copy link
Copy Markdown
Member

Closing. @howjmay If you wish to pursue this, please make another PR.

@charris charris closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg left review comments
@rgommers rgommers rgommers left review comments
@seiko2plus seiko2plus seiko2plus left review comments

Assignees

No one assigned

Labels

01 - Enhancement 57 - Close? Issues which may be closable unless discussion continued component: SIMD Issues in SIMD (fast instruction sets) code or machinery triage review Issue/PR to be discussed at the next triage meeting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@howjmay @rgommers @seiko2plus @InessaPawson @charris @seberg