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

Update multiply #34

Merged
merged 8 commits into from
Sep 22, 2020
Merged

Update multiply #34

merged 8 commits into from
Sep 22, 2020

Conversation

bjorgve
Copy link
Member

@bjorgve bjorgve commented Sep 3, 2020

Making vampyr work with current master of mrcpp

@bjorgve
Copy link
Member Author

bjorgve commented Sep 3, 2020

I'm not sure why the CIs are failing. Everything works smoothly on Woolf. Do you know whats wrong @robertodr?

@stigrj
Copy link
Contributor

stigrj commented Sep 3, 2020

We recently had to update the .travis.yml config in mrchem, looks like the same issue. Circle is complaining about a public key in the clone, don't know what that is about.

@robertodr
Copy link
Contributor

You might also want to update the version of MRCPP that is fetched (look inexternal/CMakeLists.txt)

@bjorgve
Copy link
Member Author

bjorgve commented Sep 4, 2020

If I'm not wrong it looks like it fetches mrcpp/master. But we should probably fetch a release by default

@robertodr
Copy link
Contributor

Indeed!

@bjorgve
Copy link
Member Author

bjorgve commented Sep 4, 2020

@stigrj I don't see anything different between mrchem/travis.yml and vampyr/travis.yml (apart from sudo: false). Which PR are you thinking about?

@robertodr
Copy link
Contributor

It's on MRChem's master branch: https://github.com/MRChemSoft/mrchem/blob/master/.travis.yml

@bjorgve bjorgve force-pushed the update_multiply branch 2 times, most recently from 94244ca to e340983 Compare September 4, 2020 10:41
@robertodr
Copy link
Contributor

Not sure what's wrong with CircleCI here... Something about access to the repo, even thought it downloads anonymously...

@robertodr
Copy link
Contributor

I've updated the Docker image, so you should now use: - image: quay.io/metamr/circleci_ubuntu-18.04-conda:41efbc8 in config.yml However, this doesn't fix the problem either, maybe @stigrj needs to reauthorize CircleCI or something...
`

@stigrj
Copy link
Contributor

stigrj commented Sep 20, 2020

I can reproduce the aborted child error from Travis in debug

@stigrj
Copy link
Contributor

stigrj commented Sep 20, 2020

I get the same abortion with master using mrcpp-1.2.0 in debug, so it's probably not related to this PR

@stigrj
Copy link
Contributor

stigrj commented Sep 20, 2020

Should vampyr always point to master, or should we pin it to versions?

@stigrj
Copy link
Contributor

stigrj commented Sep 20, 2020

I get double free or corruption errors for the Eigen::MatrixXds in ScalingBasis when compiled in debug through vampyr. If I make them pointers and let them leak, then the tests pass.

@robertodr
Copy link
Contributor

I'd say we should pin the dependency. I'll try running through the debugger later to see what's going on with the double free skullduggery...

@robertodr
Copy link
Contributor

The debugger did not provide illumination. I've compiled on sandel and the double free occurs when doing phi_tree = vp.vampyr3d.FunctionTree(MRA) It could be the binding is not 100% correct when it comes to inheritance... I'm looking into it, but I'm not sure I can fix it quickly.

@stigrj
Copy link
Contributor

stigrj commented Sep 21, 2020

I narrowed it down to a single line in the simplest case

basis = vp.InterpolatingBasis(5)

So just initialization and destruction of a ScalingBasis, in particular one of its three Eigen::MatrixXd members.

@robertodr
Copy link
Contributor

robertodr commented Sep 21, 2020 via email

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #34 into master will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
- Coverage   85.81%   85.73%   -0.08%     
==========================================
  Files          30       30              
  Lines         578      575       -3     
==========================================
- Hits          496      493       -3     
  Misses         82       82              
Impacted Files Coverage Δ
vampyr/math/math.h 100.00% <100.00%> (ø)
vampyr/functions/GaussExp.h 82.35% <0.00%> (-2.65%) ⬇️
vampyr/functions/GaussFunc.h 72.72% <0.00%> (-2.28%) ⬇️
vampyr/operators/BSOperator.h 75.00% <0.00%> (ø)
vampyr/operators/PHOperator.h 75.00% <0.00%> (ø)
vampyr/operators/ABGVOperator.h 100.00% <0.00%> (ø)
vampyr/operators/PoissonOperator.h 100.00% <0.00%> (ø)
vampyr/operators/HelmholtzOperator.h 100.00% <0.00%> (ø)
vampyr/operators/IdentityConvolution.h 33.33% <0.00%> (ø)

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 89ac77a...ae5fd31. Read the comment docs.

@stigrj
Copy link
Contributor

stigrj commented Sep 22, 2020

So there was a missing -march=native flag. I'm deja vu-ing

@robertodr
Copy link
Contributor

The agony! That flag should be exported as part of the MRCPP target then. Or removed altogether?

@robertodr
Copy link
Contributor

Can you make codecov happy?

@robertodr
Copy link
Contributor

FYI, MRChemSoft/mrcpp#147 does the export of the flag with the target.

Copy link
Contributor

@robertodr robertodr left a comment

Choose a reason for hiding this comment

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

What a trip this "simple" change was 😸 I'll push some other changes to the bindings as follow-up.

@robertodr
Copy link
Contributor

Now that all bots are happy, we can happily merge. Thanks @bjorgve!

@robertodr robertodr merged commit d40befd into MRChemSoft:master Sep 22, 2020
@bjorgve
Copy link
Member Author

bjorgve commented Sep 23, 2020

Wow you guys did allot of work here! Thanks!

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