-
Notifications
You must be signed in to change notification settings - Fork 9
Make DftFunctionals.jl types GPU compatible #23
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
base: master
Are you sure you want to change the base?
Conversation
mfherbst
left a comment
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.
The thing with the identifer and Val identifier is very hackish. Is it not possible to directly make the struct always isbits. The notion to use symbols for this is not set in stone. This points to me to bigger problems in the design of DftFnuctionals. We should discuss this and then directly change whatever is needed to help the GPU implementation.
Some workaround is necessary to calculate the derivative of a functional wrt its parameters, since ForwardDiff expects functions to take Arrays as input (see runtests.jl and change_parameters dispatching in individual functional implementations)
If that can be hidden in a convenient way, I'm ok with the change from ComponentArrays to NamedTuple. The main reason for using ComponentArrays in the first place was that it makes such AD calls more convenient and intuitive. But again, this is not set in stone. Using NamedTuple should just not make AD very cumbersome. @niklasschmitz can probably comment on this.
src/functionals/gga_c_pbe.jl
Outdated
| PbeCorrelation(parameters, pbe.lda) | ||
| end | ||
| end | ||
| # Change functional parameters based on an array of values. Assumes consistent ordering. |
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.
What do you mean by assumes consistent ordering ? I would not assume that the keys are in the same order ... as this cannot be enforced in practice.
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.
Also can we not use a ComponentArray here ?
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.
In any case I would settle on one way of getting and setting the parameters and avoid the duplication of the interface. Otherwise this may derail. DftFunctionals is anyway already too complicated for what it's doing, so if we need to refactor parts of it to make GPU work and reshuffle functionality, that's ok.
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.
Also can we not use a ComponentArray here ?
Yes, we can. We could have a single definition of change_parameters that accepts anything that seamlessly converts to a NamedTuple (including a ComponentArray):
function change_parameters(pbe::PbeCorrelation, parameters; keep_identifier=false)
if keep_identifier
PbeCorrelation(NamedTuple(parameters), pbe.lda, pbe.identifier)
else
PbeCorrelation(NamedTuple(parameters), pbe.lda)
end
end
That would be safer, and make derivatives wrt functional parameters cleaner.
It is. Actually, the problem does not show up for LDA functionals, as the identifier is not part of the The range
Finally, we could also split the functional |
|
Relaxed functional parameters type, such that constructor can take any parameter convertible to a NamedTuple (e.g. ComponentArray). The debate on how to make a functional This way, the I would appreciate if we get this refactoring through. It is currently holding me from submitting further contributions. |
This PR is part of an effort to port DFTK's stress and response calculations to the GPU. The main issue with the current implementation is that various types are not
isbits. Any data passed to a GPU kernel must beisbits.Changes:
ComponentArraytoNamedTuple. Usage remains the same, while the new type isisbits.to_isbits()function that returns aisbitsversion of a functional. Issue is that theidentifierfield was restricted to be aSymbol, which is notisbits. The parametrization of theidentifieris extended to acceptVal, and theto_isbits()function returns a copy of the input functional with the identifier set toVal{identifier}()With these changes (and a few more, for a later PR), it is possible to run DFTK stress calculations on the GPU.
Possible points of contention/discussion:
runtests.jlandchange_parametersdispatching in individual functional implementations)NamedTuple, vs formerlyComponentArray{<:Number}. So the type of the individual parameters is not enforced. I am not 100% sure whether that is an issue.