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

Fix Forte [Forte Part] #290

Merged
merged 8 commits into from
Mar 27, 2022

Conversation

JonathonMisiewicz
Copy link
Contributor

Description

This PR makes changes so that Psi can do DIIS on an ambit.BlockedTensor, as required by the forte plugin. This PR will not work until this ambit PR and this Psi PR are merged, but passed locally on a slightly outdated version of Forte.

  • As a temporary fix to Forte Compilation Error due to DF RMP2 #288, this PR replaces the OpenMP commands that cause runtime problems for me, locally. I'm happy to investigate Forte Compilation Error due to DF RMP2 #288 more with York, once this PR is on.
  • The meaning of "UNRELAXED DIPOLE" changed from dipole magnitude to dipole matrix. The corresponding "UNRELAXES DIPOLE X" and friends variables are removed. This synchronized dipole variables with their meanings in Psi.
  • We now apply DIIS to ambit.BlockedTensor directly, with much cleaner code.
  • Forte is no longer responsible for exposing ambit.BlockedTensor to Python. We now import whatever ambit exposes for us.

@JonathonMisiewicz JonathonMisiewicz changed the title Hattrick Fix Forte [Forte Part] Mar 26, 2022
Copy link
Contributor

@lcyyork lcyyork left a comment

Choose a reason for hiding this comment

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

I am OK with the PR, thanks!

Copy link
Member

@fevangelista fevangelista left a comment

Choose a reason for hiding this comment

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

Thanks for all these changes. With the new DIIS code things are much more compact. I left a few comments. Let's first merge the ambit and psi4 changes and then merge this in forte.

@@ -70,7 +70,6 @@ using namespace pybind11::literals;
namespace forte {

// see the files in src/api for the implementation of the following methods
void export_ambit(py::module& m);
Copy link
Member

Choose a reason for hiding this comment

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

It's great to see code removed!

setup.py Outdated Show resolved Hide resolved
tests/pytest/ambit/test_ambit.py Show resolved Hide resolved
@JonathonMisiewicz
Copy link
Contributor Author

JonathonMisiewicz commented Mar 26, 2022

Ambit and Psi PRs are in, so tests should pass.

@codecov
Copy link

codecov bot commented Mar 27, 2022

Codecov Report

Merging #290 (5b8dcf4) into master (15f5f2a) will increase coverage by 6.14%.
The diff coverage is 72.54%.

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   59.59%   65.74%   +6.14%     
==========================================
  Files         201      198       -3     
  Lines       50564    50170     -394     
==========================================
+ Hits        30135    32985    +2850     
+ Misses      20429    17185    -3244     
Impacted Files Coverage Δ
forte/mrdsrg-spin-integrated/mrdsrg.h 0.00% <ø> (ø)
forte/orbital-helpers/mp2_nos.cc 55.40% <25.00%> (ø)
forte/mrdsrg-spin-integrated/mrdsrg_amplitude.cc 46.67% <50.00%> (+19.28%) ⬆️
forte/mrdsrg-spin-adapted/sa_mrdsrg_amps.cc 74.68% <75.00%> (+74.68%) ⬆️
forte/api/forte_python_module.cc 92.37% <100.00%> (ø)
forte/casscf/casscf.cc 70.99% <100.00%> (+1.41%) ⬆️
forte/casscf/mcscf_2step.cc 89.34% <100.00%> (+1.37%) ⬆️
forte/mrdsrg-spin-adapted/sa_mrdsrg_diis.cc 100.00% <100.00%> (+100.00%) ⬆️
forte/mrdsrg-spin-integrated/dsrg_mrpt2.cc 64.22% <100.00%> (+0.02%) ⬆️
forte/mrdsrg-spin-integrated/dsrg_mrpt3.cc 86.84% <100.00%> (+<0.01%) ⬆️
... and 44 more

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 15f5f2a...5b8dcf4. Read the comment docs.

Copy link
Member

@fevangelista fevangelista left a comment

Choose a reason for hiding this comment

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

Wow, thanks for all this hard work! Happy to see the green checkmark back 😄.

@fevangelista fevangelista merged commit 2e722ea into evangelistalab:master Mar 27, 2022
@JonathonMisiewicz JonathonMisiewicz deleted the hattrick branch March 28, 2022 18:26
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