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

median changes original array #17153

Closed
ranjanan opened this issue Jun 27, 2016 · 9 comments
Closed

median changes original array #17153

ranjanan opened this issue Jun 27, 2016 · 9 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior maths Mathematical functions

Comments

@ranjanan
Copy link
Contributor

ranjanan commented Jun 27, 2016

julia> a = rand(10,10)
10×10 Array{Float64,2}:
 0.588313  0.696375   0.272158   0.604004   0.721866   0.788268   0.728738    0.320052   0.584744  0.865045
 0.98761   0.0869793  0.773938   0.91684    0.863999   0.943742   0.0580978   0.487495   0.961992  0.390288
 0.189591  0.62363    0.157625   0.954888   0.49841    0.941533   0.412334    0.78788    0.223935  0.829246
 0.337661  0.637544   0.348461   0.746178   0.192524   0.0180271  0.0591129   0.649086   0.658684  0.789615
 0.548669  0.858003   0.705537   0.838853   0.971037   0.0630057  0.0746857   0.202656   0.311941  0.680795
 0.443942  0.994116   0.0369483  0.655085   0.0806701  0.259669   0.537003    0.767103   0.886493  0.42927 
 0.450888  0.464264   0.76202    0.494407   0.618808   0.732945   0.311039    0.0803125  0.2154    0.236799
 0.510151  0.326896   0.0835976  0.25796    0.335705   0.646855   0.00134084  0.517298   0.395151  0.625445
 0.707753  0.982399   0.325194   0.0679382  0.311107   0.469233   0.830239    0.845385   0.649784  0.365174
 0.360456  0.680518   0.581434   0.7556     0.840826   0.99433    0.301829    0.275632   0.918035  0.932225

julia> median(a,1)
1×10 Array{Float64,2}:
 0.480519  0.659031  0.336828  0.700632  0.558609  0.6899  0.306434  0.502396  0.617264  0.65312

julia> a
10×10 Array{Float64,2}:
 0.189591  0.0869793  0.0369483  0.0679382  0.0806701  0.0180271  0.00134084  0.0803125  0.2154    0.236799
 0.337661  0.326896   0.0835976  0.25796    0.192524   0.0630057  0.0580978   0.202656   0.223935  0.365174
 0.360456  0.464264   0.157625   0.494407   0.311107   0.259669   0.0591129   0.275632   0.311941  0.390288
 0.443942  0.62363    0.272158   0.604004   0.335705   0.469233   0.0746857   0.320052   0.395151  0.42927 
 0.450888  0.637544   0.325194   0.655085   0.49841    0.646855   0.301829    0.487495   0.584744  0.625445
 0.510151  0.680518   0.348461   0.746178   0.618808   0.732945   0.311039    0.517298   0.649784  0.680795
 0.548669  0.696375   0.581434   0.7556     0.721866   0.788268   0.412334    0.649086   0.658684  0.789615
 0.588313  0.858003   0.705537   0.838853   0.840826   0.941533   0.537003    0.767103   0.886493  0.829246
 0.707753  0.982399   0.76202    0.91684    0.863999   0.943742   0.728738    0.78788    0.918035  0.865045
 0.98761   0.994116   0.773938   0.954888   0.971037   0.99433    0.830239    0.845385   0.961992  0.932225

Notice that a has now changed.

versioninfo():

Julia Version 0.5.0-dev+4956
Commit 861100c* (2016-06-27 16:47 UTC)
Platform Info:
  System: Darwin (x86_64-apple-darwin15.4.0)
  CPU: Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.7.1 (ORCJIT, broadwell)
@ranjanan
Copy link
Contributor Author

@simonbyrne help please

@simonster simonster added the bug Indicates an unexpected problem or unintended behavior label Jun 27, 2016
@simonbyrne
Copy link
Contributor

definitely wrong. I'll fix it

@simonster
Copy link
Member

simonster commented Jun 27, 2016

Looks like an unintended consequence of #16260, which changed mapslices to pass a view instead of a copy to the mapped function. Since median across dimensions uses median! via mapslices, this ends up modifying the original array. This change to mapslices is evidently breaking and should be documented in NEWS. cc @timholy

simonbyrne added a commit that referenced this issue Jun 27, 2016
Due to a change in the behaviour of `mapslices` (#16260), `median(X,k)` would mutate the underlying array. Fixes #17153.
@simonbyrne
Copy link
Contributor

Thanks @simonster, I've updated the NEWS.md file.

@kshyatt kshyatt added the maths Mathematical functions label Jun 27, 2016
@timholy
Copy link
Member

timholy commented Jun 28, 2016

More likely, we should revert that. I don't even remember doing that intentionally.

@timholy timholy self-assigned this Jun 28, 2016
@simonbyrne
Copy link
Contributor

I don't see what's wrong with the change to mapslices: shouldn't this be quite a bit faster?

@jmxpearson
Copy link

Thanks, all. This fixes a very subtle bug that was causing Distributions tests to fail.

@simonster
Copy link
Member

@simonbyrne It might be faster sometimes but it probably won't be faster all the time. If you're mapping rows, then the resulting views may be slow if you're repeatedly accessing elements, because the elements won't be contiguous in memory. It's possible that a better general approach would be to construct a single array corresponding to the size of the region, then copy into it and reuse it for all invocations of the mapped function. That would still be breaking, but it would probably break less.

@StefanKarpinski
Copy link
Member

It is often the allocation and subsequent GC that slows things down, not necessarily the data copies.

tkelman pushed a commit that referenced this issue Jul 18, 2016
* Add test for Issue #17153 and PR #17154

* Fix whitespaces
mfasi pushed a commit to mfasi/julia that referenced this issue Sep 5, 2016
Due to a change in the behaviour of `mapslices` (JuliaLang#16260), `median(X,k)` would mutate the underlying array. Fixes JuliaLang#17153.
mfasi pushed a commit to mfasi/julia that referenced this issue Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior maths Mathematical functions
Projects
None yet
Development

No branches or pull requests

7 participants