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.

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

fix extrema(A,dim) when length(dim)>1 #22118

Merged
merged 1 commit into from
Aug 22, 2017
Merged

fix extrema(A,dim) when length(dim)>1 #22118

merged 1 commit into from
Aug 22, 2017

Conversation

bjarthur
Copy link
Contributor

title says it all.

is it too late to backport this to 0.6?

tkelman
tkelman reviewed May 30, 2017
View reviewed changes
test/reduce.jl Outdated
@test extrema(reshape(1:24,2,3,4),1) == reshape([(1,2),(3,4),(5,6),(7,8),(9,10),(11,12),(13,14),(15,16),(17,18),(19,20),(21,22),(23,24)],1,3,4)
@test extrema(reshape(1:24,2,3,4),2) == reshape([(1,5),(2,6),(7,11),(8,12),(13,17),(14,18),(19,23),(20,24)],2,1,4)
@test extrema(reshape(1:24,2,3,4),3) == reshape([(1,19),(2,20),(3,21),(4,22),(5,23),(6,24)],2,3,1)
A = circshift( reshape(1:24,2,3,4), (0,1,1))
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary space after opening paren

@bjarthur bjarthur mentioned this pull request May 30, 2017
Open
@tkelman
Copy link
Contributor

tkelman commented May 30, 2017

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@ararslan
Copy link
Member

I'm inclined to think that the slowdowns are noise.

@bjarthur
Copy link
Contributor Author

bjarthur commented Jun 5, 2017

latest push contains a CartesianRange version of extrema(A,dims) which removes the @generated function. local benchmarks show it's about the same speed as the Cartesian version, depending on the size of A and the length and value of dims.

re. nanosoldier-- half the changes were speed ups, and the other were slow downs, most by only ~15% or so. is there anything to be concerned about? note too that while extrema(A) is used in base, extrema(A,dims) is not.

@KristofferC
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@bjarthur
Copy link
Contributor Author

bjarthur commented Jun 6, 2017

extrema is not tested at all in BaseBenchmarks, so the nanosoldier results are irrelevant, no?

@KristofferC
Copy link
Member

I just saw it was ran before and changes was made so I ran it again :)

@bjarthur
Copy link
Contributor Author

bump

@timholy
Copy link
Member

timholy commented Aug 22, 2017

At some point it would be good to update this to general indices, but this looks fine to me.

@timholy timholy merged commit 91c2931 into JuliaLang:master Aug 22, 2017
ararslan pushed a commit that referenced this pull request Sep 11, 2017
@timholy @ararslan
Merge pull request #22118 from bjarthur/bja/fixextrema
7e847f9 
fix extrema(A,dim) when length(dim)>1

Ref #22118
(cherry picked from commit 91c2931)
ararslan pushed a commit that referenced this pull request Sep 13, 2017
@bjarthur @ararslan
fix extrema(A,dim) when length(dim)>1
8ad9240 
Ref #22118
(cherry picked from commit 4a470cb)
vtjnash pushed a commit that referenced this pull request Sep 14, 2017
@bjarthur @vtjnash
fix extrema(A,dim) when length(dim)>1
01f3a50 
Ref #22118
(cherry picked from commit 4a470cb)
ararslan pushed a commit that referenced this pull request Sep 15, 2017
@bjarthur @ararslan
fix extrema(A,dim) when length(dim)>1
acba61a 
Ref #22118
(cherry picked from commit 4a470cb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

@ararslan ararslan ararslan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bjarthur @tkelman @nanosoldier @ararslan @KristofferC @timholy