Skip to content

Change all Array and Vector to AbstractArray and AbstractVector#87

Open
biogeo wants to merge 1 commit into
JuliaMath:masterfrom
biogeo:abstractarray
Open

Change all Array and Vector to AbstractArray and AbstractVector#87
biogeo wants to merge 1 commit into
JuliaMath:masterfrom
biogeo:abstractarray

Conversation

@biogeo

@biogeo biogeo commented Feb 24, 2016

Copy link
Copy Markdown

Changing all Array and Vector type specifications in method signatures to AbstractArray and AbstractVector to make code more generic, as per #86.

@mlubin

mlubin commented Feb 24, 2016

Copy link
Copy Markdown
Collaborator

Could you add some tests of this functionality?

@biogeo

biogeo commented Feb 24, 2016

Copy link
Copy Markdown
Author

Sure, I'm not sure exactly what to test though. Is it reasonable to add a dependency on, e.g., DataFrames for the purposes of testing with a DataArray?

@mlubin

mlubin commented Feb 24, 2016

Copy link
Copy Markdown
Collaborator

You could add a testing dependency on DataFrames (i.e., in test/REQUIRE). You could also just test with subarrays.

@biogeo

biogeo commented Feb 24, 2016

Copy link
Copy Markdown
Author

Sounds good, will do when I have a chance.

@biogeo

biogeo commented Feb 24, 2016

Copy link
Copy Markdown
Author

I added tests, essentially by replicating existing tests using SubArrays instead of Arrays.

@biogeo

biogeo commented Feb 24, 2016

Copy link
Copy Markdown
Author

Travis tests for Julia nightly release seem to be failing, but not on a line that makes sense to me based on anything I changed.

@biogeo

biogeo commented Feb 25, 2016

Copy link
Copy Markdown
Author

I can confirm that I get the same test failure when running Pkg.test on Julia 0.5.0-dev+2440 (the current dev version on JuliaBox) on the current master branch, so I don't think the failed test is anything to do with this PR.

@pkofod

pkofod commented Dec 20, 2016

Copy link
Copy Markdown

I guess this requires changing sub to view? Or include some mechanism for supporting both v0.4 and v0.5 ?

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.

3 participants