8000 ENH: Increase average calculation performance. See #5507 by minhlongdo · Pull Request #5525 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Increase average calculation performance. See #5507 #5525

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 0 commits into from
Closed

ENH: Increase average calculation performance. See #5507 #5525

wants to merge 0 commits into from

Conversation

minhlongdo
Copy link

No description provided.

@minhlongdo minhlongdo changed the title Increase average calculation performance. See #5507 ENC Increase average calculation performance. See #5507 Jan 30, 2015
@minhlongdo minhlongdo changed the title ENC Increase average calculation performance. See #5507 EHC Increase average calculation performance. See #5507 Jan 30, 2015
@minhlongdo minhlongdo changed the title EHC Increase average calculation performance. See #5507 ECH Increase average calculation performance. See #5507 Jan 30, 2015
@minhlongdo minhlongdo changed the title ECH Increase average calculation performance. See #5507 ENH Increase average calculation performance. See #5507 Jan 30, 2015
@minhlongdo minhlongdo changed the title ENH Increase average calculation performance. See #5507 ENH: Increase average calculation performance. See #5507 Jan 30, 2015
Nodd
Nodd reviewed Jan 30, 2015
View reviewed changes
numpy/lib/function_base.py
@@ -514,8 +514,7 @@ def average(a, axis=None, weights=None, returned=False):
avg = a.mean(axis)
scl = avg.dtype.type(a.size/avg.size)
else:
a = a + 0.0
wgt = np.asarray(weights)
wgt = np.asarray(weights, dtype=float)
Copy link
Contributor

Choose a reason for hiding this comment

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

As said in the corresponding issue, you should use np.result_type(weights, np.float) instead of float only.

Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately that won't work as result_type needs an ndarray as input, which is was asarray creates

Copy link
Member

Choose a reason for hiding this comment

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

Maybe asarray needs an argument to say 'make the dtype at least this high
on the casting graph (higher is OK)'?
On 30 Jan 2015 16:50, "Julian Taylor" notifications@github.com wrote:

In numpy/lib/function_base.py
#5525 (comment):

@@ -514,8 +514,7 @@ def average(a, axis=None, weights=None, returned=False):
avg = a.mean(axis)
scl = avg.dtype.type(a.size/avg.size)
else:

  •    a = a + 0.0

  •    wgt = np.asarray(weights)

  •    wgt = np.asarray(weights, dtype=float)

unfortunately that won't work as result_type needs an ndarray as input,
which is was asarray creates

—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/5525/files#r23854432.

Copy link
Member

Choose a reason for hiding this comment

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

Guys, the calculation down below already does a result type for weights, you do not need to cast it here (unless you want at least 64bit floats). As for a, you can actually use result type to cast it as suggested, and just do the cast in the sum like it is done for the weights?

@njsmith
Copy link
Member
njsmith commented Jan 30, 2015

On further consideration, this change doesn't make much sense to me. The
speedup is from dropping the a = a + 0.0 -- how does changing the dtype of
weights replace this?

OTOH assuming that weights is real is probably OK -- I thought we were
forcing 'a' to be real.

BTW, copy=0 is the default for asarray.
On 30 Jan 2015 10:50, "Minh-Long Do" notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

#5525
Commit Summary

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#5525.

8000
@jaimefrio
Copy link
Member

I think the best solution would look something like:

wts = np.asarray(wts)
dt = np.result_type(a.dtype, wts.dtype, float)

and later on always make calls to the ufuncs with dtype=dt specified:

scl = wgt.sum(axis=axis, dtype=dt)
avg = np.multiply(a, wgt, dtype=dt).sum(axis) / scl

There is also a very weird construct a little bit further down:

if returned:
    scl = np.multiply(avg, 0) + scl

which we could probably do away with if we implemented the above, as the only conceivable reason I can see to do that would be to have scl be of np.result_type(avg.dtype, scl.dtype).

@ewmoore
Copy link
Contributor
ewmoore commented Jan 30, 2015

Doesn't np.result_type(a.dtype, wts.dtype, float) upcast float32 to
float64?

On Fri, Jan 30, 2015 at 2:55 PM, Jaime notifications@github.com wrote:

I think the best solution would look something like:

wts = np.asarray(wts)
dt = np.result_type(a.dtype, wts.dtype, float)

and later on always make calls to the ufuncs with dtype=dt specified:

scl = wgt.sum(axis=axis, dtype=dt)
avg = np.multiply(a, wgt, dtype=dt).sum(axis) / scl

There is also a very weird construct a little bit further down:

if returned:
scl = np.multiply(avg, 0) + scl

which we could probably do away with if we implemented the above, as the
only conceivable reason I can see to do that would be to have scl be of np.result_type(avg.dtype,
scl.dtype).

—
Reply to this email directly or view it on GitHub
#5525 (comment).

@seberg
Copy link
Member
seberg commented Jan 30, 2015

Yes, you would need to use np.result_type(a, wts, 0.) (or use a.dtype if you want it to never behave like a scalar for a, etc.)

@minhlongdo
Copy link
Author

In the current code base by doing a = a + 0.0 is to force a to be real. So I was wondering if np.average(a, weights=None, returned=False) also accepts an array of complex numbers. Because, if it accepts np.complex128 then the operation a = a + 0.0 would not be necessary since the imaginary and real parts of the complex number are already np.float64 . So I did the following in the if statement if not isinstance(a, np.matrix) to reduce unnecessary operations that forces a to be real.

if a.dtype is np.dtype('complex128'):
8000
 
    if weights is None:
        # Just convert into an np.array
        a = np.asarray(a)
    else:
        if a.dtype is not np.dtype('complex128'):
            # Convert to np.array and set np.dtype to float64
            a = np.asarray(a, dtype=np.dtype('float64'))
else:
    # If a is a np.matrix and there are weight values
    if weights is not None:
        # if dtype of a is not np.dtype('complex128') then make it to be np.float64
        if a.dtype is not np.dtype('complex128'):
            # No need to attempt conversion for np.float64 if it is already np.float64
            if a.dtype is not np.dtype('float64'):
                # Worse case scenario, np.matrix is not np.float64 then the following will force a to be np.float64
                a = a + 0.0

The performance of this change by using the script https://gist.github.com/skuschel/2d148a37a2ce17925fb0 provided by skuschel is the following:

execution time np.average: 3.06e-01 sec
exectution time np.mean: 3.00e-01 sec

What do you guys think?

@minhlongdo minhlongdo closed this Feb 3, 2015
@minhlongdo minhlongdo reopened this Feb 3, 2015
@minhlongdo
Copy link
Author

Sorry I accidentally closed it.

@charris
Copy link
Member
charris commented Feb 8, 2015

In the current code base by doing a = a + 0.0 is to force a to be real

Real or complex while preserving inexact type. It is an tricky way to achieve that end. Maybe we should just check for inexact? This PR looks a bit too complicated to me as it stands.

@minhlongdo At some point the commits will need to be squashed into one and the commit message cleaned up. See doc/source/dev/gitwash/development_workflow.rst for discussion of commit messages.

@minhlongdo
Copy link
Author

@charris Will do, thanks for the feedback.

@minhlongdo minhlongdo closed this Feb 10, 2015
@minhlongdo
Copy link
Author

@charris I looked into inexact and it does indeed simply the proposed solution.
I somehow managed to mess up, so I made a new pull request #5551. I am sorry for the mess.

@jaimefrio jaimefrio mentioned this pull request Feb 10, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@minhlongdo @njsmith @jaimefrio @ewmoore @seberg @charris @Nodd @juliantaylor
0