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

Upgrade to Julia 0.7/1.0 #34

Merged
merged 2 commits into from
Aug 13, 2018
Merged

Upgrade to Julia 0.7/1.0 #34

merged 2 commits into from
Aug 13, 2018

Conversation

acroy
Copy link
Owner

@acroy acroy commented Aug 12, 2018

Addresses JuliaLang/julia#33 : First I was trying to stay compatible to 0.6, but it was too much hassle. So with this PR support for versions < 0.7 is dropped.

@@ -3,7 +3,6 @@ os:
- linux
- osx
julia:
- 0.6
- 0.7
- nightly
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also add 1.0 here.

src/expmv.jl Outdated
warn("opnorm($(typeof(A)), Inf) is not defined, fall back to using `anorm = 1.0`.
To suppress this warning, please specify the anorm parameter manually.")
@warn "opnorm($(typeof(A)), Inf) is not defined, fall back to using `anorm = 1.0`.
To suppress this warning, please specify the anorm keyword manually."
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to 1.0 compatibility, but the second mention of anorm could have backquotes around it

@test res < 1e-6

#= commented since this test takes sometime difs very long
Copy link
Contributor

Choose a reason for hiding this comment

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

this is new. do these tests take longer on v0.7 than previously? what does difs mean?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sorry! Yes, this took much longer, but I didn't investigate what causes it. ("difs" is just a weird typo) Since the other tests for chbv are Ok, I took this one out.

Copy link
Contributor

@garrison garrison left a comment

Choose a reason for hiding this comment

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

Everything generally looks good to me. I looked carefully at the main code and only skimmed the changes to the tests. I agree it makes most sense to drop support for julia 0.6. Many packages are doing this already, and this package is a special case as well since it ties so heavily into the multiplication framework.

Once this is merged, it would be nice to see how performance under julia 0.7/1.0 compares with the previous commit run under julia 0.6. Is there any overall change in speed? Related to #21.

@garrison
Copy link
Contributor

garrison commented Aug 12, 2018

Also note that there are a few warnings under CI:

┌ Warning: opnorm(LinearOp, Inf) is not defined, fall back to using `anorm = 1.0`.
│ To suppress this warning, please specify the anorm keyword manually.
└ @ Expokit ~/build/acroy/Expokit.jl/src/expmv.jl:8

EDIT: sorry, I had forgotten that this warning is intentionally generated by this package; carry on.

@garrison
Copy link
Contributor

garrison commented Aug 12, 2018

Regarding benchmarking, my (nonscientific) results indicate that the tests take ~70% longer to run on julia 0.7 (even with this pull request). I'm not even sure that the culprit is within Expokit.jl itself, but it would be nice to get to the bottom of this ... EDIT: perhaps related to JuliaLang/LinearAlgebra.jl#545

@acroy
Copy link
Owner Author

acroy commented Aug 12, 2018

I agree that we should do a comparison under 0.6 and 0.7/1.0. The question is if we should merge this anyways to get rid of deprecation warnings etc and open a new issue to track the possible regression?

@mforets
Copy link
Contributor

mforets commented Aug 12, 2018

I suggest that you merge and request to tag a new release, adding in the notes that this version drops support for v0.6; note that different versions can be benchmarked with

julia> benchmarkpkg("Expokit", tagged-version-or-branch)

@acroy acroy merged commit 4f91a8a into master Aug 13, 2018
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