Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests #17079

Sacha0 · 2016-06-23T22:33:52Z

Second step towards JuliaLang/LinearAlgebra.jl#231 (for a description of the overall objective and plan, see #17066 (comment)). This pull request replaces all full(X) calls with convert(Array, X) and accordingly migrates tests and documentation. Thanks and best!

tkelman · 2016-06-24T15:56:36Z

doc/manual/arrays.rst

@@ -811,7 +811,7 @@ reference.
 +----------------------------------------+----------------------------------+--------------------------------------------+
 | :func:`speye(n) <speye>`               | :func:`eye(n) <eye>`             | Creates a *n*-by-*n* identity matrix.      |
 +----------------------------------------+----------------------------------+--------------------------------------------+
-| :func:`full(S) <full>`                 | :func:`sparse(A) <sparse>`       | Interconverts between dense                |
+| :func:`convert(Array, S) <convert>`                 | :func:`sparse(A) <sparse>`       | Interconverts between dense                |


table alignment needs fixing

Fixed. Thanks!

andreasnoack · 2016-07-10T01:42:55Z

@Sacha0 Thanks for all the work on this. With #17066 merged, I guess it's time for a rebase here.

…ide base/sparse and base/linalg), apart from the no-op fallback for `AbstractArray`s in abstractarray.jl.

…ir and test/linalg. Migrate `full` to `convert` in some documentation.

tkelman · 2016-07-16T12:05:45Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

tkelman · 2016-07-16T12:16:08Z

test/sparsedir/sparse.jl

-    @test full(ss116[i,[end-3:-2:1;]]) == aa116[i,[end-3:-2:1;]]
-    @test full(ss116[[end-3:-2:1;],j]) == aa116[[end-3:-2:1;],j]
+    @test convert(Array, ss116[i,[3:2:end-3;]]) == aa116[i,[3:2:end-3;]]
+    @test convert(Array, ss116[[3:2:end-3;],j]) == aa116[[3:2:end-3;],j]


It's a bad sign when your diffs get long enough that github gives up on coloring words any more. I should probably have looked at this commit-by-commit.

Apologies. How should I improve this? Thanks!

no action needed, what you did with splitting to multiple commits was good, I just should have reviewed in the same way. or we can complain to github to highlight more of long diffs

nanosoldier · 2016-07-16T14:38:47Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

Sacha0 · 2016-07-21T02:57:43Z

Travis i686 timeout unrelated?

Sacha0 · 2016-07-21T13:59:23Z

Thanks for the review and merge!

tkelman · 2016-07-22T14:46:57Z

This looks like it had some unintended consequences, ref JuliaStats/DataArrays.jl#208 - not sure what that was doing before, I guess specializing full to return something other than Array ?

tkelman · 2016-07-22T16:23:33Z

Since this isn't really critical and wasn't supposed to have visible consequences, I think we may want to revert it for now and revisit after branching 0.5?

Sacha0 · 2016-07-22T16:54:09Z

Since this isn't really critical and wasn't supposed to have visible consequences, I think we may want to revert it for now and revisit after branching 0.5?

Sounds good! Prior to merger I assumed this was 0.6-dev material. Taking a look now at the base/abstractarray.jl code @andreasnoack identified (independent of reversion) and will report back. Thanks!

Sacha0 · 2016-07-22T21:13:37Z

As @andreasnoack noted above, full in the generic concatenation methods' similar(full(X), T, dims) calls seems intended to handle annotation and special matrix types such as Diagonal and LowerTriangular and otherwise be a no-op. Specifically, when X is such an annotation or special matrix type, the full call returns a form of X that enables similar(full(X), T, dims) to return something concatenable yet, if possible, more similar to X than Array. On the other hand, the presence of full seems to make the generic concatenation methods inapplicable to sparse matrices/vectors and perhaps similar types.

One path forward would be to introduce backingtype and change the identified calls to similar(convert(backingtype(X), X), T, dims). But backingtype would need to exist for a broad swathe of types, practically requiring introduction of a no-op backingtype fallback. And that no-op backingtype fallback would essentially be a reintroduction of full's no-op fallback with a different name and slightly clearer definition, though still fraught with issues. (E.g., what should backingtype return for special matrix types? Need you abstract backingtype to something that provides the closest type that can have arbitrary shape and is fully writeable to make similar(convert(backingtype(X), X), T, dims) work as needed here? That sounds like a more principled replacement for full, but also sounds suspiciously like one conception of similar.) Moreover, in some sense this path would couple the logic of annotation and special matrix types with many unrelated things, whereas reducing that coupling was part of the appeal of deprecating full.

Another path forward would be removing full from the identified similar(full(X), T, dims) calls. This approach has the appeal of decoupling annotation and special matrix types' logic from the rest of the codebase. In the near term this approach would reduce the degree to which the generic concatenation methods preserve structure. But writing specialized similar methods could restore much of that ability, and specialized concatenation methods (as presently exist for e.g. sparse matrices/vectors due to the fulls in question) could take care of the rest. This approach would make clarifying the meaning of similar (and consequently convert etc) more urgent, but that might be a good thing.

Thoughts? Did the full calls serve any other purpose? Best!

This was referenced Jun 24, 2016

Towards #12251 and #12153, reimplement all full(X) method defs. as convert(Array, X) and add convert(AbstractArray, X) methods for Factorizations #17066

Merged

WIP: Towards #12251 part 3, deprecate full and remove related exports and docs #17082

Closed

tkelman reviewed Jun 24, 2016
View reviewed changes

Sacha0 force-pushed the repfull branch 2 times, most recently from b928fa2 to a937904 Compare June 27, 2016 21:34

andreasnoack added the linear algebra Linear algebra label Jul 10, 2016

Sacha0 force-pushed the repfull branch from a937904 to 082a43c Compare July 15, 2016 04:39

Replace full(X) calls with convert(Array, X) throughout base/sparse.

4dd7355

Sacha0 force-pushed the repfull branch from 082a43c to 8bf1968 Compare July 15, 2016 23:49

Sacha0 added 5 commits July 15, 2016 17:20

Replace full(X) calls with convert(Array, X) throughout base/linalg.

bd3c3ac

Replace full(X) calls with convert(Array, X) elsewhere in base/ (outs…

a4e3cd2

…ide base/sparse and base/linalg), apart from the no-op fallback for `AbstractArray`s in abstractarray.jl.

Migrate full(X) to convert(Array, X) in tests in test/sparsedir.

17fd169

Migrate full(X) to convert(Array, X) in tests in test/linalg.

6f0915f

Migrate full(X) to convert(Array, X) in tests outside of test/sparsed…

cd24d3f

…ir and test/linalg. Migrate `full` to `convert` in some documentation.

Sacha0 force-pushed the repfull branch from 8bf1968 to cd24d3f Compare July 16, 2016 00:26

tkelman reviewed Jul 16, 2016
View reviewed changes

tkelman merged commit b82d923 into JuliaLang:master Jul 21, 2016

Sacha0 deleted the repfull branch July 21, 2016 13:59

tkelman mentioned this pull request Jul 22, 2016

test/sort.jl broken on current Julia master JuliaStats/DataArrays.jl#208

Closed

tkelman mentioned this pull request Jul 22, 2016

Revert "Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests" #17564

Merged

Sacha0 mentioned this pull request Jul 28, 2016

Remove full from similar(full(X), T, dims) calls in generic concatenation methods #17660

Merged

Sacha0 mentioned this pull request Aug 7, 2016

Fix #16519 by adding missing convert methods #17848

Merged

Sacha0 mentioned this pull request Sep 14, 2016

Use a name other than full for returning original array from a factorization JuliaLang/LinearAlgebra.jl#229

Closed

Sacha0 mentioned this pull request Oct 8, 2016

Replace all full(X) calls with convert(Array, X) and migrate tests #18850

Closed

Sacha0 mentioned this pull request Aug 15, 2017

the semantics of storage[type] #23270

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests #17079

Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests #17079

Sacha0 commented Jun 23, 2016 •

edited

Loading

tkelman Jun 24, 2016

Sacha0 Jun 24, 2016

andreasnoack commented Jul 10, 2016

tkelman commented Jul 16, 2016

tkelman Jul 16, 2016

Sacha0 Jul 19, 2016

tkelman Jul 19, 2016

nanosoldier commented Jul 16, 2016

Sacha0 commented Jul 21, 2016

Sacha0 commented Jul 21, 2016

tkelman commented Jul 22, 2016

tkelman commented Jul 22, 2016

Sacha0 commented Jul 22, 2016

Sacha0 commented Jul 22, 2016

Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests #17079

Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests #17079

Conversation

Sacha0 commented Jun 23, 2016 • edited Loading

tkelman Jun 24, 2016

Choose a reason for hiding this comment

Sacha0 Jun 24, 2016

Choose a reason for hiding this comment

andreasnoack commented Jul 10, 2016

tkelman commented Jul 16, 2016

tkelman Jul 16, 2016

Choose a reason for hiding this comment

Sacha0 Jul 19, 2016

Choose a reason for hiding this comment

tkelman Jul 19, 2016

Choose a reason for hiding this comment

nanosoldier commented Jul 16, 2016

Sacha0 commented Jul 21, 2016

Sacha0 commented Jul 21, 2016

tkelman commented Jul 22, 2016

tkelman commented Jul 22, 2016

Sacha0 commented Jul 22, 2016

Sacha0 commented Jul 22, 2016

Sacha0 commented Jun 23, 2016 •

edited

Loading