Skip to content

Fix deprecation warnings on v0.6#6

Merged
ChrisRackauckas merged 1 commit into
SciML:masterfrom
ScottPJones:spj/fixdep06
May 10, 2017
Merged

Fix deprecation warnings on v0.6#6
ChrisRackauckas merged 1 commit into
SciML:masterfrom
ScottPJones:spj/fixdep06

Conversation

@ScottPJones

Copy link
Copy Markdown
Contributor

No description provided.

@codecov-io

codecov-io commented May 9, 2017

Copy link
Copy Markdown

Codecov Report

Merging #6 into master will increase coverage by 5.38%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage    52.7%   58.08%   +5.38%     
==========================================
  Files           3        3              
  Lines         148      136      -12     
==========================================
+ Hits           78       79       +1     
+ Misses         70       57      -13
Impacted Files Coverage Δ
src/array_partition.jl 50.81% <0%> (+8.35%) ⬆️
src/vector_of_array.jl 64.51% <0%> (+3.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19c0532...52f7d20. Read the comment docs.

Comment thread src/array_partition.jl Outdated
Base.:*(A::ArrayPartition, B::Number) = ArrayPartition((x .* B for x in A.x)...)
Base.:/(A::ArrayPartition, B::Number) = ArrayPartition((x ./ B for x in A.x)...)
Base.:\(A::Number, B::ArrayPartition) = ArrayPartition((x ./ A for x in B.x)...)
import Base: +, -, *, /, \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, I prefer the other way these days. I seems less prone to shadowing errors.

@ChrisRackauckas

Copy link
Copy Markdown
Member

Can you re-enable nightlies on Travis / AppVeyor (keep it allowed failure though). I'd like to see this does get rid of the depwarns.

@ScottPJones ScottPJones force-pushed the spj/fixdep06 branch 3 times, most recently from ecd60d5 to 04ec67a Compare May 9, 2017 18:08
Comment thread src/array_partition.jl Outdated
Base.:./(A::ArrayPartition, B::Number) = ArrayPartition((x ./ B for x in A.x)...)
Base.:.\(A::Number, B::ArrayPartition) = ArrayPartition((x ./ A for x in B.x)...)
end
VERSION < v"0.6.0-dev.1614" && include_string("""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this can be done with @static if rather than include_string(), right?

@ScottPJones ScottPJones May 9, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, even with @static it has to be parsed, and these warnings are coming directly from the Scheme parsing code, not from deprecations.jl.
(that was actually the first thing I tried ;-) )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that's what I thought too. Have you actually tried it? Adding @static consistently stops the deprecation warnings in v0.6 for me.

@rdeits rdeits May 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Err, didn't notice that you said you tried it, sorry!

Then I'm super confused, because I just tried it now and it worked.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Specifically, I changed the code to:

@static if VERSION < v"0.6-"
  Base.:.+(A::ArrayPartition, B::ArrayPartition) = ArrayPartition((x .+ y for (x,y) in zip(A.x,B.x))...)
  <other methods omitted>
end

and got no deprecation warnings after changing the code and re-importing it in a new Julia session.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait, so is the include_string necessary? I'd like to hear back from @ScottPJones first before merging. I would've assumed it wasn't necessary, and can't find a simple test where it is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as far as I can tell, it's not necessary, but yeah, I'm curious why @ScottPJones and I are getting different results

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It didn't work for me (on Travis) without using include_string. (I wouldn't have gone to such a convoluted method if it had worked on Travis). In the past, I'd had to use the technique of including a small file that had only the v0.5 or v0.6 specific code, Tim Holy suggested using include_string here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: it had seemed to work for me also, just using @static, but then I still got the warnings on Travis.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I see that

https://travis-ci.org/JuliaDiffEq/RecursiveArrayTools.jl/jobs/230412103

Not sure why. Odd that it worked locally but not on Travis. While this is slightly more convoluted, it's harmless.

@ScottPJones

Copy link
Copy Markdown
Contributor Author

(everything was passing, just rebased to pick up rdeits changes for my testing)

@ChrisRackauckas ChrisRackauckas merged commit 4cebe4f into SciML:master May 10, 2017
@ScottPJones ScottPJones deleted the spj/fixdep06 branch May 10, 2017 15:12
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