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

deprecate zero-arg Matrix constructors (#14201) #20330

Merged
merged 2 commits into from
Feb 2, 2017
Merged

Conversation

StefanKarpinski
Copy link
Member

No description provided.

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Jan 30, 2017
@StefanKarpinski StefanKarpinski force-pushed the sk/rm-0arg-mat branch 2 times, most recently from 7051640 to 8903443 Compare January 30, 2017 21:32
@JeffBezanson
Copy link
Member

Need to pick between this and #20250. I vote for deprecating the zero-arg constructors; Matrix() does not seem very meaningful to me.

test/arrayops.jl Outdated
@@ -178,7 +178,6 @@ end
@test convert(Array{Int}, a) == a
@test convert(Array{Float64}, a) == a
@test convert(Matrix, a) == a
@test convert(Matrix{Int}, a) == a
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this removed? still seems valid to me

Copy link
Member Author

Choose a reason for hiding this comment

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

accident

test/arrayops.jl Outdated
@test size(Matrix()) == (0,0)

# TODO: will throw MethodError after 0.6 deprecations are deleted
@test_throws ErrorException Matrix{Int}()
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a good idea to test depwarns like this, --depwarn=error doesn't always apply, for example if running with JULIA_CPU_CORES=1

Copy link
Member Author

Choose a reason for hiding this comment

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

Then how do you recommend doing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably leave it out for now unless we want to add a depwarn_exec.jl file that we run in a separate process with known --depwarn settings.

if JULIA_CPU_CORES=1 make test-arrayops passes on this branch then this might be fine

else
error("unexpected depwarn value")
end
@test_throws MethodError Array{Int,3}()
Copy link
Member Author

Choose a reason for hiding this comment

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

We could have a @test_deprecated macro that does this for you. Usage would be something like this:

@test_deprecated size(Matrix{Int}()) == (0,0)

which would expand to one of the following, depending on the value of Base.JLOptions().depwarn:

@test_throws ErrorException size(Matrix{Int}())
@test_warn "deprecated" size(Matrix{Int}())
@test size(Matrix{Int}()) == (0,0)

The main reason I wanted this complexity in here is so that I couldn't possibly forget to update the tests when we delete the deprecations for 1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a very good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

though we probably want to put such tests in a single place and make sure they get run regularly with all 3 values of the flag (or expand to 3 tests that modify the option in a scoped way, if that's possible inside julia)

@kshyatt kshyatt added deprecation This change introduces or involves a deprecation linear algebra Linear algebra labels Jan 31, 2017
@@ -96,7 +96,6 @@ include("subarray.jl")
(::Type{Vector})() = Array{Any,1}(0)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to deprecate Vector() as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would too but I have a suspicion it's fairly widely used, so I wanted to do the uncontroversial part first. It's fairly common to initialize a vector with zero elements and there was a period where [] created a Void eltype empty array. There's also a consistency argument here: if we deprecate that we should also deprecate Vector{T}() – are you in favor of that as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

@stevengj stevengj Jan 31, 2017

Choose a reason for hiding this comment

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

I grepped for Vector() or Vector{Foo}() in all registered packages, and you're right that quite a few packages use it:

ASTInterpreter,AWS,AbstractTables,ApproxFun,Bio,BlackBoxOptim,Bukdu,CBOR,Clipper,Clustering,Compat,ControlCore,ControlSystems,Cxx,DSGE,DWARF,DataCubes,DataFrames,DeterministicPolicyGradient,DifferentialDynamicProgramming,Diversity,Dopri,EMIRT,GUITestRunner,Gallium,Graft,Hecke,IntervalWavelets,Isotonic,JuliaFEM,LLVM,LightGraphs,LightGraphsExtras,Lumberjack,Mamba,MetadataTools,Metamath,MultidimensionalTables,MultipleTesting,NearestNeighbors,NetworkFlows,NullableArrays,ODBC,ODEInterface,ObjFileBase,OpenDSSDirect,ParallelAccelerator,ParserCombinator,PlotRecipes,PlyIO,Primes,QuantumLab,RData,ResettableStacks,RigidBodyDynamics,RouletteWheels,SALSA,SemidefiniteModel,SemidefiniteModels,StaticArrays,StructuredQueries,SugarBLAS,Sundials,TensorDecompositions,TerminalUI,TestRunner,ThermodynamicsTable,TimeSeries,TimeSeriesIO,TravelingSalesmanHeuristics,Turing,TypedTables,Voronoi,WiltonInts84,WordNet

Copy link
Member

Choose a reason for hiding this comment

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

A much smaller number of packages use Matrix() or Matrix{T}():

Bio,DSGE,DifferentialDynamicProgramming,Elemental,JPLEphemeris,JuliaFEM,LightGraphs,Mamba,NearestNeighbors,OpenDSSDirect,ParallelAccelerator

Copy link
Member

Choose a reason for hiding this comment

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

I'd also favor deprecating Vector() and Vector{T}(), but doing so in a separate PR due to the expected controversy sounds like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have femtocleaner would this be a less controversial change?

Copy link
Member

Choose a reason for hiding this comment

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

See #22717

@StefanKarpinski StefanKarpinski merged commit 3c20d76 into master Feb 2, 2017
@StefanKarpinski StefanKarpinski deleted the sk/rm-0arg-mat branch February 2, 2017 17:41
@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017
@tkelman tkelman removed the needs news A NEWS entry is required for this change label May 14, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants