Skip to content

Conversation

ScottPJones
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented May 9, 2017

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.

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
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
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
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
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?

Copy link
Contributor Author

@ScottPJones ScottPJones May 9, 2017

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
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.

Copy link
Contributor

@rdeits rdeits May 9, 2017

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
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
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
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
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
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
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
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