Add Hessian calculation support #10
Conversation
|
|
||
| # add your own definition | ||
| l = length(system) * length(position(system, 1)) | ||
| return zeros(l,l) * (u"eV" / u"Å"^2) |
There was a problem hiding this comment.
This definition of hessian is inconsistent with how we return forces I believe? Forces are AbstractVector{<: SVector} and hence hessians should be AbstractMatrix{<: SMatrix}. The conversion from this format to a AbstractMatrix{<: Number} requires the specification of how positions are converted into degrees of freedom.
What is a key difficulty here is that this refers exclusively to the hessian w.r.t. positions. Hessian w.r.t. combined cell and position will need to be discussed.
|
Thanks for starting this. I think a hessian should be an |
|
Forces are In my opinnion we should use this definition to define the For cell vector Hessian, it can be build from position Hessian. Secondly we don't have AtomsBase system structure that uses fractional coordinates. |
|
thanks for reminding me about this. I am strongly opposed to using inconsistent formats here. If forces are vector(vector) then hessians must be matrix(matrix). reinterpret doesn't really play a role here in my view.
I have not seen this before. My first instinct is that this cannot be true: Take a unit cell containing a single atom. Force will always be zero, hence hessian will always be zero. But energy-volume curve will have non-zero variation so cell-hessian will be non-zero. Please text me on Zulip and explain if you think I'm wrong. |
| function generate_calculator_hessian(calc_type) | ||
| q = quote | ||
| function AtomsCalculators.calculate(::AtomsCalculators.Hessian, system, calculator::$calc_type; kwargs...) | ||
| h = AtomsCalculators.hessian(system, calculator; kwargs...) | ||
| return ( hessian = h, ) | ||
| end | ||
| end | ||
| return q | ||
| end | ||
|
|
||
| function generate_hessian(calc_type) | ||
| q = quote | ||
| function AtomsCalculators.hessian(system, calculator::$calc_type; kwargs...) | ||
| h = AtomsCalculators.calculate(AtomsCalculators.Hessian(), system, calculator; kwargs...) | ||
| return h[:hessian] | ||
| end | ||
| end | ||
| return q | ||
| end |
There was a problem hiding this comment.
There is a lot of code duplication in this file for the generates. Can this not be simplified somehow by an @eval inside a for loop or similar ?
There was a problem hiding this comment.
(Just a suggestion, no need to do that now)
There was a problem hiding this comment.
Yes, I noticed that too. Although this was not an issue untill now, so it was just faster to add these two.
| # we can ignore kwargs... or use them to tune the calculation | ||
| # or give extra information like pairlist | ||
|
|
There was a problem hiding this comment.
Yes, it is. Originally the idea was to add that there, so that, if someone looks for implementation examples, there would be some "helpful comments".
| # we can ignore kwargs... or use them to tune the calculation | ||
| # or give extra information like pairlist | ||
|
|
||
| # add your own definition |
There was a problem hiding this comment.
feels like it's not needed here.
|
If we want to add Hessian to AD tools (Zygote, ForwardDiff, etc.), we need to use use The other issue is that For cell vector Hessian you are correct. My version would only work, if atoms don't see their own images. |
|
Re AD - I don't think this is true, I've been using AD tools regularly with arrays of arrays. I think you are confusing two different hessians. For linear algebra you need the hessian w.r.t. degrees of freedom. Here we are implementing the hessian with respect to positions. They are not always the same. If we are going |
|
I'd like to return to this and propose a compromise. In JuLIP I actually had two hessian implementations: (1) pos_hessian and (2) hessian. The first was the block-matrix format that I prefer for consistency reasons. The second was with respect to DOFs. We can provide simple conversion routines. A final point: Hessians are expensive to compute. The conversion will almost never be the overhead. Maybe for some some very simple empirical potentials, but in those cases we should definitely not perform global AD. I actually do not believe that we will ever compute the global hessian using ForwardDiff. We will compute the local site-energy hessian using ForwardDiff and then write that into the global hessian. Therefore there is no conversion overhead as suggested above. |
|
based on discussions in other repos, can we pleaes add prototypes for |
Hessian calculation is defined the same way as other calculation types
@generate_interfacemacro can be used to help define the calculator interface. Also,test_hessianfunction has been added to help testing the interface.So, basically this is a very simple addition of Hessian to the interface.
@cortner, @mfherbst, @rkurchin, @jgreener64