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

Dynamical matrix for local term #752

Merged
merged 21 commits into from
Dec 21, 2023
Merged

Dynamical matrix for local term #752

merged 21 commits into from
Dec 21, 2023

Conversation

epolack
Copy link
Collaborator

@epolack epolack commented Oct 17, 2022

Open points that can be clarified after:

  • Docs of new sternheimer
  • Docs of compute_δHψ_sα

@epolack epolack force-pushed the phonon branch 4 times, most recently from f902467 to 8dd68d5 Compare October 18, 2022 19:55
antoine-levitt

This comment was marked as resolved.

@epolack epolack force-pushed the phonon branch 7 times, most recently from 2e1f42a to e067ed0 Compare November 22, 2022 16:02
@epolack epolack changed the title [WIP] Phonon computations Dynamical matrix for local term Feb 21, 2023
@mfherbst
Copy link
Member

Timings are great, I agree. No need for the wrapper.

src/fft.jl Outdated
Comment on lines 65 to 67
function irfft(basis::PlaneWaveBasis{T}, f_fourier::AbstractArray) where {T}
real(ifft(basis, f_fourier))
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd be curious why @antoine-levitt , but not blocking for the merge.

Copy link
Member

@mfherbst mfherbst left a comment

Choose a reason for hiding this comment

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

Final nits

@epolack
Copy link
Collaborator Author

epolack commented Dec 19, 2023

Actually, no it does not work. If x is an array, then this will fail (e.g., ERROR: LoadError: MethodError: Cannot convert an object of type Array{Float64, 3} to an object of type Float64). I tried to play around to change the function to something similar and as general, but I could not think of anything.

@mfherbst
Copy link
Member

Yeah, but then just add another method on AbstractVector{<:Real} or does that not do the trick ?

@epolack
Copy link
Collaborator Author

epolack commented Dec 19, 2023

No because of ForwardDiff. The encapsulation of complex is a bit tricky. I am looking if I manage to solve this.

@epolack
Copy link
Collaborator Author

epolack commented Dec 19, 2023

Ok, seems to work with promote_type.

@mfherbst
Copy link
Member

I found a parallelisation issue, that I need to fix and actually a way to completely avoid the convert_enforced.

We also need to check GPU support (will do once the CPU CI is green).

@epolack

This comment was marked as resolved.

@antoine-levitt
Copy link
Member

How is real_enforced better than enforce_real? Also beware that lowpass_for_symmetry is possibly slow

@epolack
Copy link
Collaborator Author

epolack commented Dec 20, 2023

How is real_enforced better than enforce_real? Also beware that lowpass_for_symmetry is possibly slow

real_enforced was to match convert_enforced, but as it was removed, I will revert back the rename.

@mfherbst
Copy link
Member

mfherbst commented Dec 20, 2023

GPU CI did not go through cleanly (precompilation failed, so I have no proper output from the run). Do you guys have access to a CUDA GPU or should I debug ?

@epolack
Copy link
Collaborator Author

epolack commented Dec 21, 2023

I ran the two GPU tests fine locally, so not sure what to debug. The iron one is a bit strange because of error LOBPCG is badly failing to keep the vectors normalized; this should never happen that pops-up at random.
The only think we touch GPU-wise on this PR is the removal of to_cpu functions in local.jl because [f for _ in stuff] should take care of this.

(Also, maybe it should be written somewhere that CUDA.set_runtime_version!(v"11.x") is mandatory per Libxc requirement?)

@mfherbst mfherbst merged commit ecdfa73 into JuliaMolSim:master Dec 21, 2023
@mfherbst
Copy link
Member

Ok, so I trust the GPU CI goes through. Let's see.

@epolack epolack deleted the phonon branch January 8, 2024 10:09
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