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

Implement ADC equations on the python side #71

Merged
merged 11 commits into from
Dec 3, 2020
Merged

Implement ADC equations on the python side #71

merged 11 commits into from
Dec 3, 2020

Conversation

mfherbst
Copy link
Member

@mfherbst mfherbst commented Jun 6, 2020

Still at a very early stage. Just to stash some work ...

Done in #77:

Done in this PR:

  • Rebase to pick up on Move MTMs, diffdms and transition dms to python #77
  • Rebase to pick up AdcMatrix block-wise tests #79
  • Rebase to pick up Implement singles guesses in python #98
  • Rebase to pick up State-to-state transition densities #91
  • Matrix block-wise applies
    • ADC(0)
    • ADC(1)
    • ADC(2)
    • ADC(2)-x
    • ADC(3)
    • CVS-ADC(0)
    • CVS-ADC(1)
    • CVS-ADC(2)
    • CVS-ADC(2)-x
    • CVS-ADC(3)
  • Timers on pyside routines
  • Compare timings
  • Restructure adc expressions: Make the same function generate ph_pphh and pphh_ph blocks
    (because they should always be equivalent due to ADC matrix being Hermitian)
    ➡️ Nah, causes issues with block-wise apply
  • Pass for some simple optimisations:
    • Store more (cheap) intermediates in hot applies
  • Pythonise AmplitudeVector
  • Pythonise AdcMatrix
  • Docstrings
  • Cleanup:
    • Find deprecated usage of blocks, "s", "d" etc.
    • Outdated stuff in docs

Not so clear stuff:

  • Opinions on hf.b.cv versus b.cv versus hf.b.Cv ??
  • [ ] Quick-access like functionality for T2 amplitudes and density matrices? ➡️ postponed
  • [ ] Quick-access like functionality for other things of LazyMp? ➡️ postponed

@mfherbst mfherbst force-pushed the try_adc2 branch 3 times, most recently from 838ff19 to 8424a90 Compare June 7, 2020 11:42
adcc/kernel/__init__.py Outdated Show resolved Hide resolved
@mfherbst mfherbst added adccore Related to adccore library and interface to it feature New feature or request labels Jun 15, 2020
@mfherbst mfherbst mentioned this pull request Jun 16, 2020
@maxscheurer
Copy link
Member

So should we try to get the stuff in that you already checked above? You could make an extra PR and I can review there?

@mfherbst
Copy link
Member Author

That's what I'm doing. Will take a few mins to get everything sorted out.

@mfherbst mfherbst force-pushed the try_adc2 branch 6 times, most recently from 3425486 to 9b40e66 Compare June 17, 2020 20:15
setup.cfg Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Sep 9, 2020

This pull request introduces 2 alerts when merging 9b40e66 into 60e201c - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Nov 24, 2020

This pull request introduces 2 alerts when merging c5860e5 into af7bdff - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Nov 25, 2020

This pull request introduces 1 alert when merging a7888bd into e302303 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Nov 27, 2020

This pull request introduces 1 alert when merging cde1657 into d6adad9 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2020

This pull request introduces 3 alerts when merging ef73fac into 3afd0bb - view on LGTM.com

new alerts:

  • 1 for __eq__ not overridden when adding attributes
  • 1 for Unused local variable
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2020

This pull request introduces 4 alerts when merging 6727ab1 into 3afd0bb - view on LGTM.com

new alerts:

  • 1 for __eq__ not overridden when adding attributes
  • 1 for Unused local variable
  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@mfherbst
Copy link
Member Author

mfherbst commented Nov 29, 2020

@maxscheurer Now would probably a good time to test this on some bigger cases. I ran a few tests with the compare_implementations.py script and these are my results (with 2 threads, 5 takes and 30 repeats if you want to reproduce). The time shown is the time per apply. First column is the C++ implementation, second column this new python implementation, next are the absolute and percentage changes relative to C++.

#
#-- water cc-pvtz
#
nocc = 10   nvirt = 106
full       adc2    18.97 ms      17.89 ms =    -1.07 ms    -5.66%
full   cvs-adc2    22.24 ms <    27.73 ms =     5.49 ms    24.67%
full      adc2x    60.86 ms      60.13 ms =    -0.73 ms    -1.20%
full  cvs-adc2x    58.58 ms <    60.50 ms =     1.92 ms     3.28%
full       adc3    79.15 ms      76.12 ms =    -3.03 ms    -3.83%
full   cvs-adc3   149.82 ms      73.89 ms =   -75.93 ms   -50.68%

#
#-- furane cc-pvdz
#
nocc = 36   nvirt = 144
full       adc2   163.60 ms     144.70 ms =   -18.90 ms   -11.55%
full   cvs-adc2    59.69 ms <    60.39 ms =     0.71 ms     1.18%
full      adc2x   921.28 ms     900.98 ms =   -20.30 ms    -2.20%
full  cvs-adc2x   293.75 ms <   295.82 ms =     2.07 ms     0.70%
full       adc3  1053.22 ms    1015.08 ms =   -38.14 ms    -3.62%
full   cvs-adc3  2730.74 ms     399.03 ms = -2331.71 ms   -85.39%

#
#-- ch2nh2 cc-pvtz
#
nocc = 17   nvirt = 215   unrestricted
full       adc0     1.02 ms <     1.29 ms =     0.27 ms    26.24%
full   cvs-adc0     1.00 ms <     1.24 ms =     0.24 ms    23.72%
full       adc1    10.23 ms <    11.11 ms =     0.88 ms     8.64%
full   cvs-adc1     4.30 ms <     4.52 ms =     0.22 ms     5.03%
full       adc2   322.69 ms     293.08 ms =   -29.61 ms    -9.18%
full   cvs-adc2   194.76 ms <   196.67 ms =     1.90 ms     0.98%
full      adc2x  3210.40 ms    3131.25 ms =   -79.15 ms    -2.47%
full  cvs-adc2x  1763.66 ms    1755.50 ms =    -8.16 ms    -0.46%
full       adc3  3597.51 ms    3473.48 ms =  -124.03 ms    -3.45%
full   cvs-adc3  6114.51 ms    2063.04 ms = -4051.47 ms   -66.26%

I'll add a case to try an unrestricted reference now as well.

As clearly visible there were quite a few low-hanging fruits to capture for CVS-ADC(3), which turn out to be quite worth picking. Could be of interest for @t-fransson in the future.

@maxscheurer
Copy link
Member

I'm already running a test with PNA, adc2x/aug-cc-pvdz. I can kick off some tests with your script tomorrow. 👍🏻

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2020

This pull request introduces 1 alert when merging ee36366 into 3afd0bb - view on LGTM.com

new alerts:

  • 1 for Unused import

Copy link
Member

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

Only 600 lines (net addition) of code to get ADC to Python... Wow! 🚀
I'm still running the tests, but I think we can merge this PR once you're happy with it.
Can you squash some of the commits together?

@mfherbst
Copy link
Member Author

mfherbst commented Dec 2, 2020

Yes and if you count the implicit C++ code we can remove ...

I think I have about one more evening of coding and then one of cleanup and then it should be good.

@mfherbst mfherbst changed the title Implement more equations on the python side Implement ADC equations on the python side Dec 2, 2020
@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2020

This pull request introduces 2 alerts when merging d523af6 into 3afd0bb - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2020

This pull request introduces 1 alert when merging 7a612e5 into 3afd0bb - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Dec 3, 2020

This pull request introduces 1 alert when merging 5ab7208 into 3afd0bb - view on LGTM.com

new alerts:

  • 1 for Unused import

@mfherbst mfherbst marked this pull request as ready for review December 3, 2020 19:20
@maxscheurer
Copy link
Member

I will give this a quick look, then it's g2g.

@mfherbst
Copy link
Member Author

mfherbst commented Dec 3, 2020

@maxscheurer Any updates on the timings?

Otherwise looks good to go from my end! Feel free to re-review or merg.

@mfherbst
Copy link
Member Author

mfherbst commented Dec 3, 2020

Oh and picking up your earlier comments ... it only adds a net of 350 lines after all 😄 (and there are quite a few deprecation hints we can remove soon).

@maxscheurer
Copy link
Member

Just updated the timings (aug-cc-pvdz on 2 cores), and they look good 👍

Copy link
Member

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

I approve again. 👍 Thanks for the hard, great work!

@maxscheurer maxscheurer merged commit fc9b48e into master Dec 3, 2020
@maxscheurer maxscheurer deleted the try_adc2 branch December 3, 2020 19:46
@maxscheurer maxscheurer mentioned this pull request Dec 3, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adccore Related to adccore library and interface to it feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants