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

Cubeprop improvements #1138

Merged
merged 23 commits into from Sep 6, 2018

Conversation

Projects
None yet
6 participants
@PeterKraus
Contributor

PeterKraus commented Aug 16, 2018

Description

Added a couple of new features to cubeprop. The functionality seems to work, but the PR is rather unpolished. An illustration of me while writing this PR can be found here:

https://ih0.redbubble.net/image.511550362.9563/flat,1000x1000,075,f.u1.jpg

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Developer Interest
    • Implements printing of ECP electrons, issue #1136
  • User-Facing for Release Notes
    • Added keyword "FRONTIER_ORBITALS" to cubeprop tasks. This computes the alpha HOMO and LUMO (if nalpha == nbeta), or the highest-energy doubly occupied (DOMO), the singly occupied (SOMO) and the unoccupied (LUMO) orbitals of both spins.
    • Added keyword "DUAL_DESCRIPTOR" to cubeprop tasks. Computes (for closed shell systems) the dual descriptor function (ie. the difference of the Fukui functions) from the HOMO and LUMO. Some references: 10.1021/jp046577a and 10.1007/s10910-014-0437-7.

Checklist

Status

  • Ready for review
  • Ready for merge

PeterKraus added some commits Aug 14, 2018

Added "FRONTIER_ORBITALS" to cubeprop tasks.
Computes cubefiles for HOMO and LUMO for closed-shell systems (nalpha == nbeta),
computes DOMO, SOMO and LUMO for open-shell systems (nalpha > beta).
Show outdated Hide outdated psi4/src/psi4/libcubeprop/csg.cc
Show outdated Hide outdated psi4/src/psi4/libcubeprop/csg.cc
Show outdated Hide outdated psi4/src/psi4/libcubeprop/csg.cc
Show outdated Hide outdated psi4/src/psi4/libcubeprop/cubeprop.cc
Show outdated Hide outdated psi4/src/psi4/libcubeprop/csg.cc
Show outdated Hide outdated psi4/src/psi4/libcubeprop/csg.cc
@TermeHansen

This comment has been minimized.

Show comment
Hide comment
@TermeHansen

TermeHansen Aug 18, 2018

Could we also add possibility to get valence electron density - So basically as the Dt cube file produced when atoms with ECP electrons exist.

TermeHansen commented Aug 18, 2018

Could we also add possibility to get valence electron density - So basically as the Dt cube file produced when atoms with ECP electrons exist.

@fevangelista

This comment has been minimized.

Show comment
Hide comment
@fevangelista

fevangelista Aug 18, 2018

Contributor

Really love the "FRONTIER_ORBITALS" feature. Another one that might be nice to have is "ACTIVE_ORBITALS" for printing active orbitals in CASSCF computations.

Contributor

fevangelista commented Aug 18, 2018

Really love the "FRONTIER_ORBITALS" feature. Another one that might be nice to have is "ACTIVE_ORBITALS" for printing active orbitals in CASSCF computations.

@loriab

This comment has been minimized.

Show comment
Hide comment
@loriab

loriab Aug 18, 2018

Member

Would be good to add new options here and here which turns into manual

Member

loriab commented Aug 18, 2018

Would be good to add new options here and here which turns into manual

@loriab

Looks good on the whole. There's some unaddressed comments, so I don't want those to be overlooked and merged.

double** v_tp = v_t->pointer();
auto v = std::make_shared<Vector>(npoints_);
double* vp = v->pointer();
add_orbitals(&v_tp[0], C2);

This comment has been minimized.

@PeterKraus

PeterKraus Aug 29, 2018

Contributor

@dgasmith is this what you had in mind?

@PeterKraus

PeterKraus Aug 29, 2018

Contributor

@dgasmith is this what you had in mind?

@PeterKraus

This comment has been minimized.

Show comment
Hide comment
@PeterKraus

PeterKraus Aug 29, 2018

Contributor

@fevangelista I am absolutely clueless about CAS methods, so I'd rather not go there.

@TermeHansen If I understand correctly, you'd like a "VALENCE_DENSITY" keyword, which plots the density due to just the valence electrons? I don't think it's terribly hard to implement, but I'd rather do that in a separate PR.

Contributor

PeterKraus commented Aug 29, 2018

@fevangelista I am absolutely clueless about CAS methods, so I'd rather not go there.

@TermeHansen If I understand correctly, you'd like a "VALENCE_DENSITY" keyword, which plots the density due to just the valence electrons? I don't think it's terribly hard to implement, but I'd rather do that in a separate PR.

@TermeHansen

This comment has been minimized.

Show comment
Hide comment
@TermeHansen

TermeHansen Aug 29, 2018

That is fine. I have some test cases with chargemol where I believe it can be very useful. Please ping me on the PR

TermeHansen commented Aug 29, 2018

That is fine. I have some test cases with chargemol where I believe it can be very useful. Please ping me on the PR

@PeterKraus PeterKraus referenced this pull request Sep 2, 2018

Merged

fmt: clang-format libcubeprop #1210

2 of 3 tasks complete

@loriab loriab referenced this pull request Sep 2, 2018

Open

format c-side codebase #1144

34 of 46 tasks complete
@loriab

loriab approved these changes Sep 3, 2018

Thanks for the ping – I had missed that there were new commits. Thanks for the new functionality, lgtm. At some point, it'd be good to add a test. There's a compare_cubes function, so it's not as formidable as it sounds. I won't merge immediately, in case @dgasmith wanted to follow up on his comments.

@loriab

This comment has been minimized.

Show comment
Hide comment
@loriab

loriab Sep 3, 2018

Member

@PeterKraus, afraid you'll have to rebase to get #1198 in to get Travis to pass.

Member

loriab commented Sep 3, 2018

@PeterKraus, afraid you'll have to rebase to get #1198 in to get Travis to pass.

@dgasmith

This comment has been minimized.

Show comment
Hide comment
@dgasmith

dgasmith Sep 5, 2018

Member

This looks good to me, but does need some tests. Examples can be found i cubeprop/*, I would borrow from these.

Member

dgasmith commented Sep 5, 2018

This looks good to me, but does need some tests. Examples can be found i cubeprop/*, I would borrow from these.

@dgasmith

This comment has been minimized.

Show comment
Hide comment
@dgasmith

dgasmith Sep 6, 2018

Member

Sorry to be nitpick, but could you call the test cubeprop-frontier to describe the type of test rather than the molecule used? This will be good to go then.

Member

dgasmith commented Sep 6, 2018

Sorry to be nitpick, but could you call the test cubeprop-frontier to describe the type of test rather than the molecule used? This will be good to go then.

@dgasmith dgasmith merged commit 2f23252 into psi4:master Sep 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@loriab loriab added this to the Psi4 1.3 milestone Sep 6, 2018

@PeterKraus PeterKraus deleted the PeterKraus:cubeprop_improvements_01 branch Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment