[MRG] MNT: Use fmax when finding the maximum#12005
Merged
qinhanmin2014 merged 1 commit intoscikit-learn:masterfrom Sep 5, 2018
Merged
[MRG] MNT: Use fmax when finding the maximum#12005qinhanmin2014 merged 1 commit intoscikit-learn:masterfrom
fmax when finding the maximum#12005qinhanmin2014 merged 1 commit intoscikit-learn:masterfrom
Conversation
Instead of adding an `if` to check for values that become the new max, simply use `fmax` to get the maximum and update the value. This improves readability. It may improve performance as `fmax` can be a single assembly instruction. Though most compilers can probably figure this out anyways.
fmax when finding the maximumfmax when finding the maximum
jnothman
approved these changes
Sep 4, 2018
Member
jnothman
left a comment
There was a problem hiding this comment.
I would be surprised if cython did not translate this usage of python max into fmax, fwiw
Contributor
Author
|
Was wondering that too, but found Cython translate it into a comparison like the one written. For example, Cython takes this function... cpdef float my_max(float a, float b) nogil:
return max(a, b)...and turns the __pyx_t_1 = __pyx_v_b;
__pyx_t_2 = __pyx_v_a;
if (((__pyx_t_1 > __pyx_t_2) != 0)) {
__pyx_t_3 = __pyx_t_1;
} else {
__pyx_t_3 = __pyx_t_2;
}
__pyx_r = __pyx_t_3;(There's of course more C code that Cython generates that has been snipped for clarity.) |
fmax when finding the maximumfmax when finding the maximum
Member
|
Have you submitted a PR to cython, then??
…On Wed, 5 Sep 2018 at 09:52, jakirkham ***@***.***> wrote:
Was wondering that too, but found Cython translate it into a comparison
like the one written. For example, Cython takes this function...
cpdef float my_max(float a, float b) nogil:
return max(a, b)
...and turns the max into this.
__pyx_t_1 = __pyx_v_b;
__pyx_t_2 = __pyx_v_a;
if (((__pyx_t_1 > __pyx_t_2) != 0)) {
__pyx_t_3 = __pyx_t_1;
} else {
__pyx_t_3 = __pyx_t_2;
}
__pyx_r = __pyx_t_3;
(There's of course more C code that Cython generates that has been snipped
for clarity.)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#12005 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6x30eX6Tt5sedOhp4I1Hz0tYqLv_ks5uXxI2gaJpZM4WZsK2>
.
|
qinhanmin2014
approved these changes
Sep 5, 2018
Contributor
Author
|
Nope. I can understand why they would want We could ask them about special casing |
Member
|
Well, that's what I meant: they should be able to check statically if they
have a pair of floats. Would C compilers never optimise this case?
|
jnothman
pushed a commit
to jnothman/scikit-learn
that referenced
this pull request
Sep 6, 2018
Instead of adding an `if` to check for values that become the new max, simply use `fmax` to get the maximum and update the value. This improves readability. It may improve performance as `fmax` can be a single assembly instruction. Though most compilers can probably figure this out anyways.
jnothman
pushed a commit
to jnothman/scikit-learn
that referenced
this pull request
Sep 17, 2018
Instead of adding an `if` to check for values that become the new max, simply use `fmax` to get the maximum and update the value. This improves readability. It may improve performance as `fmax` can be a single assembly instruction. Though most compilers can probably figure this out anyways.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of adding an
ifto check for values that become the new max, simply usefmaxto get the maximum and update the value. This improves readability. It may improve performance asfmaxcan be a single assembly instruction. Though most compilers can probably figure this out anyways.