Skip to content

Commit

Permalink
Fix some deprecations
Browse files Browse the repository at this point in the history
  • Loading branch information
@mkborregaard
mkborregaard committed Jun 7, 2017
1 parent f097fb5 commit 3a2ee0f
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 8 deletions.
2 changes: 1 addition & 1 deletion REQUIRE
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
julia 0.6
julia 0.6-pre

RecipesBase
PlotUtils 0.4.1
Expand Down
11 changes: 10 additions & 1 deletion src/Plots.jl
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
__precompile__(true)
__precompile__(false)

module Plots

Expand Down Expand Up @@ -118,6 +118,15 @@ ignoreNaN_extrema(x) = Base.extrema(x)

# ---------------------------------------------------------

# to cater for block matrices, Base.transpose is recursive.
# This makes it impossible to create row vectors of String and Symbol with the transpose operator.
# This solves this issue, internally in Plots at least.

Base.transpose(x::Symbol) = x
Base.transpose(x::String) = x

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman Jun 10, 2017

Contributor

this is type piracy, not okay, and not local to plots at all - method extension is globally visible and you're overwriting the deprecation here

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 10, 2017 •
edited
Loading

Author Member

I'll remove this because you request it.
But help me understand - Why is it not OK? There is no way this should be able to break things, as you cannot define sensibly a transpose on e.g. a Symbol. It's clearly a misfunctionality in Base that transpose recursiveness passes to things like Strings – for Numbers it has this behaviour, i.e. it just returns the number. But I guess that the reason there is a problem is that this is in a situation of change, https://github.com/JuliaLang/julia/issues/20978 and https://github.com/JuliaLang/julia/issues/21037

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman Jun 10, 2017 •
edited
Loading

Contributor

It's clearly a misfunctionality in Base that transpose recursiveness passes to things like Strings

No, it's reflective of the fact that transpose is a linear algebra operation, as discussed in those issues and the ones that deprecated the fallback. Other packages won't see the deprecation or error if they happen to import Plots while it has this method defined. You should be using permutedims (edit: or reshape) for non-linear-algebraic types.

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 10, 2017

Author Member

Well at least it indicates that the linear algebra use of transpose has been given primacy in julia. That's unfortunate for me but of course a question of priorities.

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman Jun 10, 2017

Contributor

It's been given the concise syntax. The non-linear-algebraic operation is not transpose, it's a different operation and now has different syntax. Andreas' comment at the end of https://github.com/JuliaLang/julia/issues/13171#issuecomment-238980815 never got a response. JuliaLang/julia#19344 (comment) covers similar ground.

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 11, 2017 •
edited
Loading

Author Member

I noticed Andreas' question didn't get a response. Neither did Tom's question in the comment just above, even though that question was important in my view: was there any use case where the fallback could in practice be unsafe - when removing the fallback broke so important functionality for so many, it would be more compelling if there was an actual use case where it would be unsafe. Julia, after all, is also being used outside of the realm of linear algebra, and I think that's a good thing.

I now it makes no sense to argue with you, because you did not make the decision - you simply enforce it for metadata, and there's not much point in shooting the messenger. But - I and most other contributors gain nothing personally or professionally (directly at least) from contributing to julia infrastructure. We do it because we like the language and want to see it grow.

I've removed the transpose functions completely, as requested, and refactored the code that relied upon them.

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman Jun 11, 2017

Contributor

The fallback is unsafe because it's frequently silently incorrect for custom types.

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 11, 2017

Author Member

I want to add one last thing, though, because it may aid future discussion. I can't see where you commented it, but you remarked that relying on the transpose was never a stable design decision. Honestly, that was the remark that made my motivation drop. I am sure you can see why, or we can talk about it.

But in the specific case: The core concept of Plots that you are probably remarking on here is the idea that columns are variables (plot series) whereas rows are observations (plot points). That's the basic idea of Plots, and the reason the transpose operator was useful is that it allowed to flag arguments as pertaining to variables rather than observations. It is my impression that this assocation, columns with variables and rows with observations, is quite common.

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 11, 2017

Author Member

The fallback is unsafe because it's frequently silently incorrect for custom types.

(Sorry, cross-posted.) But this would not be true for specialised fallbacks for String or Symbol, right?

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 11, 2017

Author Member

Anyway, we don't need to argue about this, I've already done everything you requested.

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman Jun 11, 2017

Contributor

Possibly not, but if those specialized fallbacks are going to be implemented anywhere, it should be in Base, not a package. "internally in Plots at least" is not an accurate description of the side effects that method extension of Base functions on Base types has.

Columns as variables and rows as observations is a convention that gets used in plenty of places, sure, but that's for data. Using the orientation of what is actually a 1-d list of items to flag whether the same argument should apply to variables vs observations seems error prone. There's an ambiguity for a single element, and with the long history behind issue 4774, the behavior of transposing a vector was clearly slated for future changes. If you have a different label per individual point across both variables and observations could warrant a 2-d array of symbols or strings, but how often would you be transposing that?

I'd use dispatch on a "RowLabels" wrapper type or separate row vs column arguments to handle the distinction, personally.

This comment has been minimized.

Sign in to view

Copy link
@mkborregaard

mkborregaard Jun 11, 2017

Author Member

Well - thanks for commenting on the design :-)

In fact, for us that work with Plots every day, it feels as if that one design decision ("columns are series") is one of those almost magical turning-points that makes the design of everything after that fall into place. I often think of it as a parallel to how in julia, multiple dispatch moved functions out of objects and that one thing changes how everything else is designed in a positive fashion.

I can positively say we haven't in practice had problems with any of the issues you are pointing out here.

But I probably won't convert you to the gospel of Plots here. I hope everything in the release should now be ready for commint.

# ---------------------------------------------------------

import Measures
import Measures: Length, AbsoluteLength, Measure, BoundingBox, mm, cm, inch, pt, width, height, w, h
const BBox = Measures.Absolute2DBox
Expand Down
4 changes: 2 additions & 2 deletions src/examples.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ PlotExample("Colors",
[:(begin
y = rand(100)
plot(0:10:100,rand(11,4),lab="lines",w=3,palette=:grays,fill=0, α=0.6)
scatter!(y, zcolor=abs(y-.5), m=(:heat,0.8,stroke(1,:green)), ms=10*abs(y-0.5)+4, lab="grad")
scatter!(y, zcolor=abs.(y-.5), m=(:heat,0.8,stroke(1,:green)), ms=10*abs.(y-0.5)+4, lab="grad")
end)]
),

Expand Down Expand Up @@ -269,7 +269,7 @@ PlotExample("Polar Plots",
"",
[:(begin
Θ = linspace(0,1.5π,100)
r = abs(0.1randn(100)+sin(3Θ))
r = abs.(0.1randn(100)+sin.(3Θ))
plot(Θ, r, proj=:polar, m=2)
end)]
),
Expand Down
2 changes: 1 addition & 1 deletion src/plot.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function plot(plt1::Plot, plts_tail::Plot...; kw...)

# build our plot vector from the args
n = length(plts_tail) + 1
plts = Array(Plot, n)
plts = Array{Plot}(n)
plts[1] = plt1
for (i,plt) in enumerate(plts_tail)
plts[i+1] = plt
Expand Down
2 changes: 1 addition & 1 deletion src/recipes.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ end
edge_x, edge_y, weights = x, y, z.surf

float_weights = float(weights)
if is(float_weights, weights)
if float_weights === weights
float_weights = deepcopy(float_weights)
end
for (i, c) in enumerate(float_weights)
Expand Down
4 changes: 2 additions & 2 deletions src/utils.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ function convert_to_polar(x, y, r_extrema = calc_r_extrema(x, y))
x = zeros(n)
y = zeros(n)
for i in 1:n
x[i] = cycle(r,i) * cos(cycle(phi,i))
y[i] = cycle(r,i) * sin(cycle(phi,i))
x[i] = cycle(r,i) * cos.(cycle(phi,i))
y[i] = cycle(r,i) * sin.(cycle(phi,i))
end
x, y
end
Expand Down

0 comments on commit 3a2ee0f

Please sign in to comment.