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

Possible bug when deltaRhoOut generated #1367

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

bhourahine
Copy link
Member

As the work array is then not used, so nothing to copy. Would impact excited state use of resulting density matrix.

As the work array is then not used, so nothing to copy. Would impact
excited state use of resulting density matrix.
Copy link

codecov bot commented Jan 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (91794c2) 70.63% compared to head (57d8281) 70.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1367   +/-   ##
=======================================
  Coverage   70.63%   70.63%           
=======================================
  Files         230      230           
  Lines       43961    43963    +2     
=======================================
+ Hits        31051    31053    +2     
  Misses      12910    12910           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@vanderhe vanderhe left a comment

Choose a reason for hiding this comment

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

Your fix seems fine, although I'm not sure if we should simultaneously store $\mathrm{P}(\mathbf{k})$ and $\Delta\mathrm{P}(\mathbf{k})$ at all. At this point they are of course still identical, but I don't see a reason why we could not store e.g. $\Delta\mathrm{P}(\mathbf{k})$ only and calculate $\mathrm{P}(\mathbf{k})$ on the fly in the linresp routines.

@bhourahine
Copy link
Member Author

Agreed, but for the moment lets just keep things working and then refactor later.

@bhourahine bhourahine merged commit 0643649 into dftbplus:main Jan 15, 2024
7 checks passed
@vanderhe
Copy link
Member

@bhourahine As far as I can see this bug only shows up for colinear spin-pol. + excited gradients, but there is no such regression test, e.g. Benzene-TD-LC-Spinpol and Benzene-TD-LC-Spinpol-Iter do not test the gradients (not included in _autotest.tag)

@bhourahine
Copy link
Member Author

@vanderhe suggests we need a regression for this, either by extending those tests or adding a new case. The Casida routines only handle $\Gamma$-point and then only for excitation energies at the moment.

@bhourahine bhourahine deleted the deltaRhoOutStore branch January 15, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants