-
Notifications
You must be signed in to change notification settings - Fork 89
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
Minor refactoring of the FFTs #996
base: master
Are you sure you want to change the base?
Conversation
Nice, thanks! I'll have more time for a proper review later, but just some comments from a quick look. Some of this I like, some not so sure. FFTBundle - > FFTPlans ? Also gvectors and rvectors do belong to the pwbasis, not just the fft thing. I kind of like just passing basis to fft, since we do this for a number of routines, and also because the normalization and interpretation do depend on the actual system, it's not just an fft, but we can discuss that. |
Of course, I am happy to discuss! Also note that, in the current state of the PR, I left |
@abussy This is a very good start. For me what has always bothered me is that the ffts and the basis are indeed super entangled, but the relationship is not always 1:1. E.g. one may have multiple fft plans in different precisions for the same basis. That is how the idea for this PR came up. In that sense I don't think it's bad to have the G_vectors and r_vectors (which are not the cartesian things at this stage) stored in the fft object and I would even go one step further and remove them from the basis. Similarly I like the idea to enforce that In that spirit I actually find it weird it has a For the name. I would even go more high-level and just call it |
OK then it's more something like a FFTGrid that handles all the operations related to that (including r and G vectors), it's more than what I had in mind but sure why not. Still not too sure about the signature of fft(), maybe if you're going to change the kpoints structure wait until then? Maybe it'll lead to some simplifications, I never really liked the fft(basis, rho) vs fft(basis, kpoint, psi) thing. Maybe if kpoint contains a reference to the fft grid we can do fft(basis.grid, rho) and fft(kpoint.grid, psi) or something? |
Also that's a potentially pretty disrupting change, so let's merge all the non breaking internal structure change first and then do the external API change in a separate PR? |
For the name, I like @antoine-levitt idea of
I personally see no problem from removing it from the basis. Then we can either define a
I agree with that. The
I am not really sure how I will tackle the kpoints structure yet, but I will keep that in mind. As for now, I would leave the |
G_vectors::T_G_vectors | ||
r_vectors::T_r_vectors | ||
# A FFTGrid containing all necessary data for FFT opertations related to this basis | ||
fft_grid::FFTGrid{T, VT, T_G_vectors, T_r_vectors} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better sth like:
fft_grid::FFTtype
and then in the basis type arguments have FFTtype <: FFTGrid{T, VT}
.
function G_vectors(fft_size::Union{Tuple,AbstractVector}) | ||
# Note that a collect(G_vectors_generator(fft_size)) is 100-fold slower | ||
# than this implementation, hence the code duplication. | ||
start = .- cld.(fft_size .- 1, 2) | ||
stop = fld.(fft_size .- 1, 2) | ||
axes = [[collect(0:stop[i]); collect(start[i]:-1)] for i = 1:3] | ||
[Vec3{Int}(i, j, k) for i in axes[1], j in axes[2], k in axes[3]] | ||
end | ||
|
||
function G_vectors_generator(fft_size::Union{Tuple,AbstractVector}) | ||
# The generator version is used mainly in symmetry.jl for lowpass_for_symmetry! and | ||
# accumulate_over_symmetries!, which are 100-fold slower with G_vector(fft_size). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these to fft.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now remember why I left them here: the Kpoint
constructor requires the G_vector(fft_size)
function. I see 2 options:
- I leave this here
- I move these to
fft.jl
, change the include order to havefft.jl
beforeKpoint.jl
. Then I need to make sure that there is no hard dependence onKpoint
infft.jl
. This is mostly ok, but for thecompute_fft_size()
function, which I am afraid should be moved toKpoint.jl
This is some kind of circle dependency, and I don't see an easy way out atm.
Edit: probably option 2) makes more sense, logically. But the name of compute_fft_size()
might need a change to reflect that this is very much linked to the choice of k-points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, there is no explicit reliance on the Kpoint
type in compute_fft_size()
. It can stay where it is. Sorry for the noise
In this PR, I propose some refactoring of the FFTs, in order to try and help disentangle the massive
PlaneWaveBasis
type.The rational behind the proposed changes is that FFTs can, in principle, take place independently of the basis. As a result, a new
FFTBundle
type is created, containing all data required to perform such operations. The creation of aFFTBundle
can take place independently of aPlaneWaveBasis
, and only requiresfft_size
,unit_cell_volume
andarchitecture
. All subsequent calls to thefft()
andifft()
functions now require aFFTBundle
as argument, rather than aPlaneWaveBasis
. Instead of having a loose collection of FFT plans, thePlaneWaveBasus
now has a singleFFTBundle
as a field.The main changes are in these files:
fft.jl
, where theFFTBundle
type is defined. All functions in this file are now independent of thePlaneWaveBasis
type. An extra couple of functions (fft_matrix()
andifft_matrix()
) were also moved to this file fromPlaneWaveBasis.jl
, as they are also basis independent.Kpoint.jl
file was created, containing the definition of theKpoint
type and related functions. This was also taken out ofPlaneWaveBasis.jl
, for the main reason thatKpoint
must be defined for FFTs, which should be defined for the creation of aPlaneWaveBasis
. Note that this also reduces the size and complexity ofPlaneWaveBasis.jl
.PlaneWaveBasis.jl
, which was simplified.The changes in all other files essentially consist in replacing argument in FFT function calls, e.g.
fft(basis, ...)
-->fft(basis.fft_bundle, ...)
. Note that the originalfft()
calls with the basis as argument could be easily retrieved by defining functions such asfft(basis, ...) = fft(basis.fft_bundle, ...)
inPlaneWaveBasis.jl
. I guess this is a stylistic question, and I personally like having these explicit calls (I am happy to change if the DFTK standard is different).In the future, I plan to open a new PR where I take some of the K-point complexity out of
PlaneWaveBasis.jl
, by creating a new type containing all the necessary data for a set K-points.