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

Fix 0.6 breakages and depwarns #57

Merged
merged 2 commits into from
Feb 21, 2017
Merged

Fix 0.6 breakages and depwarns #57

merged 2 commits into from
Feb 21, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Feb 17, 2017

I think this gets most of the recent depwarns & breakages. Will require a new Compat tag (JuliaLang/Compat.jl#329). Also doesn't fix

ArgumentError: colons must be converted by to_indices(...)
  Stacktrace:
   [1] macro expansion at /home/tim/.julia/v0.6/AxisArrays/src/indexing.jl:47 [inlined]
   [2] reaxis at /home/tim/.julia/v0.6/AxisArrays/src/indexing.jl:18 [inlined]
   [3] getindex at /home/tim/.julia/v0.6/AxisArrays/src/indexing.jl:52 [inlined]
   [4] getindex(::AxisArrays.AxisArray{Float64,2,Array{Float64,2},Tuple{AxisArrays.Axis{:Timestamp,StepRange{DateTime,Base.Dates.Hour}},AxisArrays.Axis{:Cols,Array{Symbol,1}}}}, ::Colon, ::Symbol) at /home/tim/.julia/v0.6/AxisArrays/src/indexing.jl:76
   [5] include_from_node1(::String) at ./loading.jl:539
   [6] include(::String) at ./sysimg.jl:14
   [7] include_from_node1(::String) at ./loading.jl:539
   [8] include(::String) at ./sysimg.jl:14
   [9] eval(::Module, ::Any) at ./boot.jl:235
   [10] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./REPL.jl:66
   [11] macro expansion at ./REPL.jl:97 [inlined]
   [12] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73
ERROR: LoadError: LoadError: There was an error during testing
while loading /home/tim/.julia/v0.6/AxisArrays/test/core.jl, in expression starting on line 177
while loading /home/tim/.julia/v0.6/AxisArrays/test/runtests.jl, in expression starting on line 8

Wouldn't mind a little collaboration in fixing that particular problem 😄. If anyone is interested, feel free to grab this and run with it.

@mbauman
Copy link
Member

mbauman commented Feb 17, 2017

I have something locally that hacks around to_indices. It does a lot of other, much crazier things, though. I'll push it as a separate PR… and you can take a look.

@timholy timholy changed the title WIP (do not merge): Fix 0.6 breakages and depwarns Fix 0.6 breakages and depwarns Feb 20, 2017
@timholy
Copy link
Member Author

timholy commented Feb 20, 2017

OK, I think the breakages are mostly fixed, though I noticed a couple of small issues in Base julia that needed fixing. (JuliaLang/julia#20686)

Also, I switched to Unitful for the README and tests. Most urgent because of JuliaLang/julia#20671, but I think it's also the future so we might as well switch. It passes tests locally if I have the master branch of Unitful checked out, but it doesn't on the current release. (CC @ajkeller34.)

@timholy
Copy link
Member Author

timholy commented Feb 20, 2017

OK to merge and tag? Locally, this passes on both 0.5 and 0.6 (I have master checked out on Unitful).

@mbauman
Copy link
Member

mbauman commented Feb 21, 2017

Sure, since it's a test-only dependency that makes sense. I'd prefer to not have CI broken for too long, so if a tag isn't expected for a while I'd like to Pkg.checkout Unitful on travis.

@mbauman mbauman merged commit 4d2f8cc into master Feb 21, 2017
@mbauman mbauman deleted the teh/0.6 branch February 21, 2017 00:10
@ajkeller34
Copy link
Collaborator

I hope to tag Unitful today or tomorrow, since it looks like Attobot has made that a whole lot easier.

Which reminds me, I still have that open PR on AxisArrays... sorry for the delay, I just haven't found the time. I hope to return to it soon, and am also excited to see what happens with #56 or #58.

convert(T, first(v) + (i-1)*step(v))
end
# When running with "--check-bounds=yes" (like on Travis), the bounds-check isn't elided
@inline function Base.unsafe_getindex{T}(v::Range{T}, i::Integer)
Copy link

Choose a reason for hiding this comment

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

why are these here instead of base?

Copy link
Member

Choose a reason for hiding this comment

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

Because this was easier than going back and reverting that portion of JuliaLang/julia#14957.

Copy link

Choose a reason for hiding this comment

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

what needs reverting?

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of JuliaLang/julia#14957 was to centralize/simplify, but for ranges there do turn out to be cases where you want to perform the computation even when bounds-checking is forced on. Since a certain number of unsafe_getindex calls have crept back into range.jl, maybe it's not unreasonable to add them back systematically.

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 rather have unsafe_getindex be removed completely and use a new name for the range computation since it's not really unsafe (in terms of memory or validity) at all. For this use-case we really don't want this function to apply at all to non-ranges. But this really isn't a high priority for me — unsafe_getindex is an undocumented function that's rarely used these days.

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.

4 participants