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

[release-0.5] Backport #19015 #19100

Merged
merged 4 commits into from
Nov 4, 2016
Merged

[release-0.5] Backport #19015 #19100

merged 4 commits into from
Nov 4, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 25, 2016

This is against release-0.5. CC @tkelman, @nalimilan.

@tkelman tkelman changed the title Backport #19015 [release-0.5] Backport #19015 Oct 25, 2016
@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2016

missing the pr reference in the commit message

@timholy
Copy link
Member Author

timholy commented Oct 25, 2016

Each references the "parent" commit separately (I used cherry-pick -x). Should I have done it differently? How do you recommend fixing?

@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2016

commit --amend on the last one and add "ref #19015"

@timholy
Copy link
Member Author

timholy commented Oct 25, 2016

As a preemptive strike, I just did a grep on my installed packages. It looks like the following call Base.reduced_dims and/or Base.reduced_dims0 directly:

  • DataArrays
  • Images
  • PackageEvaluator
  • StatsBase

With this PR, the return type is now an Indices tuple rather than a Dims tuple, so it's almost certain to cause a breakage.

If that's too much breakage, there's an easy fix: make the internal function _reduced_dims (which returns an Indices tuple) and then have reduced_dims unwrap any OneTo tuple as a Dims tuple. That should (theoretically) make this "invisible." Would you prefer that?

@timholy
Copy link
Member Author

timholy commented Oct 25, 2016

Alternatively, we could rename the core function reduced_indices on julia-0.6, define a deprecated reduced_dims to wrap-and-unwrap Indices<--->Dims, and then having a version of that change join this one before backporting to 0.5. The advantage of the latter strategy is that there would be some testing of this last piece on 0.6 first.

This may also make backporting to julia-0.5 safer

(cherry picked from commit 4d47670)

ref #19103
@timholy
Copy link
Member Author

timholy commented Oct 26, 2016

See #19103.

@timholy
Copy link
Member Author

timholy commented Nov 1, 2016

OK, commit message edited to link to the original PR, and #19103 added. In theory this should be 100% non-breaking.

@tkelman
Copy link
Contributor

tkelman commented Nov 4, 2016

Thanks. I'll run pkgeval on a separate PR, hopefully this won't break anything but let's see.

@tkelman tkelman merged commit 1866a0e into release-0.5 Nov 4, 2016
@tkelman tkelman deleted the teh/backport_19015 branch November 4, 2016 07:28
@timholy
Copy link
Member Author

timholy commented Nov 4, 2016

Probably want #19196 too.

@tkelman
Copy link
Contributor

tkelman commented Nov 4, 2016

That only touched deprecated.jl, which this didn't

@timholy
Copy link
Member Author

timholy commented Nov 4, 2016

Right, but the same issue might apply. I haven't checked carefully, though. (This had some of the deprecated functions without the deprecations.)

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.

2 participants