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

Make median non-mutating on arrays. #17154

Merged
merged 1 commit into from
Jun 28, 2016
Merged

Make median non-mutating on arrays. #17154

merged 1 commit into from
Jun 28, 2016

Conversation

simonbyrne
Copy link
Contributor

Due to a change in the behaviour of mapslices (#16260), median(X,k) would mutate the underlying array. Fixes #17153.

Due to a change in the behaviour of `mapslices` (#16260), `median(X,k)` would mutate the underlying array. Fixes #17153.
@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2016

How many more of these might there be?

@simonbyrne
Copy link
Contributor Author

This is the only call to mapslices in Base, but may be more in packages.

@ranjanan
Copy link
Contributor

ranjanan commented Jun 27, 2016

#16091 strikes again?

LoadError: MethodError: no method matching Base.Serializer.__deserialized_types__.##269()

@tkelman
Copy link
Contributor

tkelman commented Jun 28, 2016

mapslices turns out to be quite a bit more widely used in packages than in base.

@ranjanan
Copy link
Contributor

ranjanan commented Jun 28, 2016

@tkelman should this fix be merged? @timholy had some thoughts on the issue too. Also, does this need tests?

@JeffBezanson JeffBezanson merged commit a48676c into master Jun 28, 2016
@JeffBezanson
Copy link
Sponsor Member

Passing a mutating function to a higher-order function is always a big red flag. It happens here because this is a strange case where the function returns a new result, but is allowed to overwrite its input.

@tkelman tkelman deleted the sb/median-slice branch June 28, 2016 15:12
ranjanan added a commit to ranjanan/julia that referenced this pull request Jun 28, 2016
tkelman pushed a commit that referenced this pull request Jul 18, 2016
* Add test for Issue #17153 and PR #17154

* Fix whitespaces
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants