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.

Sign up for GitHub

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

Jump to bottom

Juliav1.0 #7

Merged
merged 12 commits into from
Jan 23, 2019
Merged

Juliav1.0 #7

merged 12 commits into from
Jan 23, 2019

Conversation

jlapeyre
Copy link
Contributor

This PR upgrades SchattenNorms.jl for Julia v1.0.

@jlapeyre
Add test-dependence on ECOS
4784474 
	new file:   test/REQUIRE
@jlapeyre
Require minimum version Julia v1.0
924975d 
	modified:   REQUIRE
@jlapeyre
Upgrade and organize tests for Julia v1.0
c7db6d0 
Note that three tests do not pass. For these tests, @test
has been replaced by @info.
	modified:   test/bench-dnorm.jl
	modified:   test/runtests.jl
	modified:   test/test-dnorm.jl
@jlapeyre
Upgrade src for Julia v1.0
17ce6b7 
This commit completes the minimal changes for SchattenNorms.jl
to compile and to run the tests.

	modified:   src/SchattenNorms.jl
	modified:   src/dnorm.jl
	modified:   src/dnormu.jl
@jlapeyre
Make docstring for dnorm visible
063e817 
The docstring was defined inside a let block. I moved the
docstring into the file scope.

	modified:   src/dnorm.jl
@jlapeyre
Remove commented out precompilation line
c538428 
Precompilation is the default now. We may need to specify that
SchattenNorms.jl *not* be compiled.

	modified:   src/SchattenNorms.jl
@jlapeyre
Use fully qualified symbols from Convex.jl
99f9560 
	modified:   src/dnorm.jl
@jlapeyre
Try to disable precompilation
684eea8 
I write "__precompile__(false)". But, this does not stop precompilation.
And precompilation fails.

	modified:   src/SchattenNorms.jl
@jlapeyre
Qualify solver name: SCS.SCSSolver()
6bc18f5 
Qualifying the name of the solver in test/test-dnorm.jl
shortens the search for its definition.

modified:   test/test-dnorm.jl
@jlapeyre
Qualify the linear algebra names with "Convex"
c301644 
* In upgrading to v1.0, the linear algebra symbols in src/dnorm.jl were
qualified with "LinearAlgebra". But, Convex has imported the names
"isposdef", "tr", etc., from LinearAlgebra and added new methods. It is
these new methods from Convex that are called here.

* Implement optionally passing a solver to `dnorm`.

	modified:   src/dnorm.jl
@jlapeyre
Change "warn" to "@warn", a required v1.0 upgrade
e4d873a 
"warn" no longer exsists. I did not add a test for this. But, if the
solver fails to converge these warnings will now be printed rather than
throwing an error.

	modified:   src/dnorm.jl
@jlapeyre
Update .travis.yml
c0d9cae
@matthewware
Copy link
Collaborator

I'll have to do some more looking around to see why the diamond test is failing...

@matthewware
Copy link
Collaborator

So I think some up-stream changes are causing some slight differences in the numerics. I was able to get all tests to pass 70% of the time. The other 30% failed in the same way with the same numerical values off by the last digit of tolerance. For me its when the computed values of the norm and the ddist are compared. I'll need to see what's changed in the Convex.jl code base...

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Nov 29, 2018 •
edited
Loading

Me too. I guess it is this one

maximal dnorm examples, random |   18     18
dnorm for Pauli channels: Test Failed at /home/lapeyre/.julia/dev/SchattenNorms/test/test-dnorm.jl:99
  Expression: isapprox(pauli_dnorm, calc_dnorm1, atol=1.0e-5)
   Evaluated: isapprox(0.61187159697893, 0.6118557639635327; atol=1.0e-5)

I see this with a few matrices occasionally. Mostly this one. And it appears to be pauli_dnorm ddist that differs by the tolerance (sometimes) from run to run.

@matthewware
Copy link
Collaborator

Ah! I totally for got about this PR. Looks like I duplicated a lot of your work with another PR. On the one hand I was able to track down the numerical issues across OSes (see: jump-dev/SCS.jl#114), but now I need to merge the two PRs.

@jlapeyre
Copy link
Contributor Author

I need to merge the two PRs.

Argh... That's unfortunate. I hope there are not too many collisions.

I was able to track down the numerical issues across OSes

Good.

I also forgot it was not merged. I'm still interested in helping; The BBN repos are probably the best QI packages in Julia. But, I'm spread a bit thin at the moment always.

@matthewware matthewware merged commit c0d9cae into BBN-Q:master Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jlapeyre @matthewware