Skip to content

Introduce test dependency on SpecialFunctions #192

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

Closed
wants to merge 1 commit into from

Conversation

ararslan
Copy link
Member

Should fix the failure noted by @yuyichao in #191.

@ararslan
Copy link
Member Author

Problem. This package supports 0.4, SpecialFunctions is 0.5 minimum. Not sure what the best thing to do would be. cc @jrevels

@jrevels
Copy link
Member

jrevels commented Feb 12, 2017

Thanks for this, I hadn't realized these functions were already moving out of Base in v0.6.

Not sure what the best thing to do would be. cc @jrevels

Well, code-wise, we could just wrap the SpecialFunctions import in the appropriate version check.

As for managing the REQUIRE...the easiest thing to do would be to deprecate ForwardDiff v0.4 support, but that's quite a heavy-handed solution. I'd only want to do if it's really necessary. The alternative is dropping SpecialFunctions from the REQUIRE and cloning it down in the Travis script, but of course this would be horrible for users trying to run tests locally (and probably PkgEval).

@tkelman might have valuable insights here.

@yuyichao
Copy link
Contributor

Can SpecialFunctions just add a 0.4 tag? (that just import a bunch of functions from Base)?

@ararslan
Copy link
Member Author

Since this is just in a test, I did the less clean thing and set it up to install the package if it's unavailable and the Julia version is after removal.

@tkelman
Copy link
Contributor

tkelman commented Feb 13, 2017

No, package code should not be invoking Pkg, use test/REQUIRE for that. One of the packages needs a branch for 0.4 support. It's not worth supporting 3 different Julia versions in a single code base.

test/DualTest.jl Outdated
@@ -7,6 +7,11 @@ using ForwardDiff: Partials, Dual, value, partials
import NaNMath
import Calculus

if VERSION >= v"0.6.0-dev.2767"
Pkg.installed("SpecialFunctions") === nothing && Pkg.add("SpecialFunctions")
Copy link
Contributor

Choose a reason for hiding this comment

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

not correct when installed elsewhere on LOAD_PATH, and does not correctly remove the package after testing if it wasn't otherwise asked for

@ararslan
Copy link
Member Author

Is the UNSUPPORTED_NESTED_FUNC stuff entirely necessary in the tests? If not, removing that would be a workable solution for getting this to work on 0.4-0.6.

@jrevels
Copy link
Member

jrevels commented Feb 13, 2017

Is the UNSUPPORTED_NESTED_FUNC stuff entirely necessary in the tests?

All supported functions have to be tested; it seems the error is thrown when testing erf(::Dual) (a test that should pass). I'm not sure how removing UNSUPPORTED_NESTED_FUNC changes anything...that's just skipping the nested differentiation test for functions in the auto-defined list whose second derivatives aren't supported (i.e. their constituent functions don't have derivative definitions in Calculus).

I haven't been following the development of SpecialFunctions, but isn't there usually a full deprecation cycle before Base functions are removed? Why does attempting to use erf throw an error instead of a depwarn?

Anyway, it seems like the simplest solution here is to just drop v0.4 support from ForwardDiff moving forward. It was bound to happen eventually anyway, so let's just do that.

@ararslan
Copy link
Member Author

their constituent functions don't have derivative definitions in Calculus

Ah, that's the part I was missing. Sorry, I don't know much about how this package actually works. (In my defense, David Sanders didn't talk about erf in his lecture!) I thought this was an internally-defined list of functions and updating to use the SpecialFunctions definitions would be fine.

isn't there usually a full deprecation cycle before Base functions are removed?

The deprecation period in these cases is different in that it's an error in Base pointing to the appropriate package for a release, then the informative Base error is removed later and it becomes an UndefVarError without the appropriate package installed. The same approach was used when primes, priority queues, etc. were moved to packages.

Anyway, it seems like the simplest solution here is to just drop v0.4 support from ForwardDiff

Yeah, probably...

It was bound to happen eventually anyway

RIP 0.4

@jrevels jrevels mentioned this pull request Feb 14, 2017
@jrevels
Copy link
Member

jrevels commented Feb 14, 2017

Alright, I dropped v0.4 support, so this PR should be a bit simpler now 😄

@ararslan
Copy link
Member Author

Awesome, thanks! Updating this PR should be as simple now as removing the second commit. I can get to that later today.

@ararslan
Copy link
Member Author

Stack overflow on erf(::Dual) now...

@jrevels
Copy link
Member

jrevels commented Feb 15, 2017

Weird, I'm not seeing it locally (running yesterday's Julia master on macOS).

@ararslan ararslan closed this Feb 15, 2017
@ararslan ararslan reopened this Feb 15, 2017
@ararslan
Copy link
Member Author

I wonder if it's related to JuliaLang/julia#20103...

@jrevels
Copy link
Member

jrevels commented Feb 23, 2017

Restarted the build again here, fingers crossed. I still can't reproduce this locally on macOS - tests pass for me on v0.5 and v0.6.

If Travis hits that StackOverflow again, we should recruit a Linux user to try this out and confirm the error.

@ararslan
Copy link
Member Author

If the cause is what I think it is, it's been fixed on master, so hopefully that's reflected in the nightly binaries.

@jrevels
Copy link
Member

jrevels commented Feb 23, 2017

Note that the same failure is occurring on v0.5 as well, it's not just a v0.6 problem.

Anyway, I'm working on a competing PR right now that will actually overload the SpecialFunction versions on v0.6 rather than overload the deprecated Base versions. It might fix this error, if the problem is just an infinitely recursive call chain.

@ararslan
Copy link
Member Author

Note that the same failure is occurring on v0.5 as well, it's not just a v0.6 problem.

If you look at the Travis log, it does appear to have been fixed on nightly--the failure there is something different.

@jrevels
Copy link
Member

jrevels commented Feb 23, 2017

If you look at the Travis log, it does appear to have been fixed on nightly--the failure there is something different.

I'm confused by what you mean - both v0.5 and nightly Travis logs seem to indicate the same error:

Testing Dual...
  ...testing Dual{0,Int64} and Dual{0,Dual{0,Int64}}
  ...testing Dual{0,Float32} and Dual{0,Dual{0,Float32}}
WARNING: Encountered error when testing erf(::Dual):
ERROR: LoadError: LoadError: StackOverflowError:
⋮

@ararslan
Copy link
Member Author

how did i miss that ._.

@yuyichao
Copy link
Contributor

Does it happen without recompiling sysimg?

@jrevels
Copy link
Member

jrevels commented Feb 23, 2017

superceded by #200

@jrevels jrevels closed this Feb 23, 2017
@ararslan ararslan deleted the aa/specfun branch February 23, 2017 19:21
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