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

ENH: Add min values comparison for floating point #18882

Merged
merged 1 commit into from
May 2, 2021

Conversation

howjmay
Copy link
Contributor

@howjmay howjmay commented May 1, 2021

Add the similar implementation for minimum value comparison.

@howjmay howjmay changed the title ENH: Add main values comparison for floating point ENH: Add min values comparison for floating point May 1, 2021
Copy link
Member

@seiko2plus seiko2plus left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

numpy/core/src/common/simd/avx2/math.h Outdated Show resolved Hide resolved
@seiko2plus seiko2plus added the component: SIMD Issues in SIMD (fast instruction sets) code or machinery label May 1, 2021
@seiko2plus
Copy link
Member

close/open to re-run travis-ci.

@seiko2plus seiko2plus closed this May 1, 2021
@seiko2plus seiko2plus reopened this May 1, 2021
@mattip
Copy link
Member

mattip commented May 2, 2021

This seems to be a follow-on PR to adding max value comparison. Which PR was that?

@mattip
Copy link
Member

mattip commented May 2, 2021

ahh, #18863

@@ -390,6 +390,8 @@ SIMD_IMPL_INTRIN_1(@intrin@_@sfx@, v@sfx@, v@sfx@)
#if @fp_only@
SIMD_IMPL_INTRIN_2(max_@sfx@, v@sfx@, v@sfx@, v@sfx@)
SIMD_IMPL_INTRIN_2(maxp_@sfx@, v@sfx@, v@sfx@, v@sfx@)
SIMD_IMPL_INTRIN_2(min_@sfx@, v@sfx@, v@sfx@, v@sfx@)
SIMD_IMPL_INTRIN_2(minp_@sfx@, v@sfx@, v@sfx@, v@sfx@)
Copy link
Member

Choose a reason for hiding this comment

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

style nit: just above there is a repeat clause for 4 values, here all 4 are spelled out explicitly. Can we unify the approach?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So let me refactor to this way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mattip
Copy link
Member

mattip commented May 2, 2021

I understand correctly that these two PRs only add the intrinsic, there will be a follow-on one to use it?

@seiko2plus
Copy link
Member

seiko2plus commented May 2, 2021

npyv_max_[f32, f64], npyv_min_[f32, f64] are usually going to be used within custom kernels since both intrinsics don't guarantees handle NaN. while npyv_minp_[f32, f64] and npyv_maxp_[f32, f64] can be used directly for optimizing numpy.fmax, numpy.fmin).

@howjmay howjmay force-pushed the simd-min branch 4 times, most recently from 9a97e80 to a70e5eb Compare May 2, 2021 07:18
Add the similar implementation for minimum value comparison.
@mattip mattip merged commit 064547d into numpy:main May 2, 2021
@mattip
Copy link
Member

mattip commented May 2, 2021

Thanks @howjmay

@howjmay howjmay deleted the simd-min branch May 2, 2021 16:48
@howjmay
Copy link
Contributor Author

howjmay commented May 2, 2021

Thank you @mattip and @seiko2plus
Would you guys mind me adding max, min for signed and unsigned integers, and floor, ceiland negate operations to simd sets?

@seiko2plus
Copy link
Member

@howjmay,
yes, that would great. feel free to add any set of intrinsic as long as it has an equivalent ufunc/umath function.

Comment on lines -320 to -324
"""
Test intrinics:
npyv_max_##SFX
npyv_maxp_##SFX
"""
Copy link
Member

Choose a reason for hiding this comment

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

I didn't see these changes during my latest review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for removing this accidentally
I will fix this in the following PR of maximum and minimum

@@ -346,6 +341,31 @@ def test_max(self):
_max = self.max(vdata_a, vdata_b)
assert _max == data_max

def test_min(self):
Copy link
Member

Choose a reason for hiding this comment

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

the following are missing too

"""
Test intrinics:
    npyv_min_##SFX
    npyv_minp_##SFX
"""

I think these changes made during your latest push

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 - Enhancement component: SIMD Issues in SIMD (fast instruction sets) code or machinery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants