Inference of Allegro implemented into Fist #2722
Conversation
src/manybody_allegro.F
Outdated
| INTEGER, DIMENSION(:), POINTER :: temp_unique_list_a, work_list, work_list2 | ||
| INTEGER, DIMENSION(:, :), POINTER :: list | ||
| REAL(KIND=dp), DIMENSION(3) :: cell_v, cvi | ||
| REAL(KIND=dp), DIMENSION(:, :), POINTER :: rwork_list |
There was a problem hiding this comment.
Is it possible to replace these POINTERs with ALLOCATABLEs?
There was a problem hiding this comment.
I have done so where possible now. In some cases such as for glob_loc_list, glob_cell_v, glob_loc_list_a, unique_list_a, I have not changed to ALLOCATABLEs as they are defined above in src/manybody_potential.F and used throughout in different routines and modules.
src/pair_potential_types.F
Outdated
| allegro%unit_coords = "" | ||
| allegro%unit_forces = "" | ||
| allegro%unit_energy = "" | ||
| allegro%unit_cell = "" |
There was a problem hiding this comment.
You can set the components just at the definition of the derived type itself. Do you than still need this routine?
There was a problem hiding this comment.
I am setting the defaults in the definition of the derived type for both Allegro and NequIP, but would like to keep this as well as other routines in src/pair_potential_types.F for consistency and overall clarity, since they are called for the different pair potentials and manybody potentials implemented before.
src/pair_potential_types.F
Outdated
| ! ************************************************************************************************** | ||
| !> \brief Creates the ALLEGRO potential type | ||
| !> \param allegro ... | ||
| !> \author Teodoro Laino [teo] 11.2005 |
There was a problem hiding this comment.
Replace it with your name.
There was a problem hiding this comment.
Done both for NequIP and Allegro, thanks. In general, should one claim authorship of a routine even if it was only slightly modified from a previous one, as it was the case, for example, for all the create, copy, release, clean routines in src/pair_potential_types.F for each manybody or pair potential?
There was a problem hiding this comment.
Because we have the git logs, there is not necessarily the need to add the authorship but if you do, just add a hint that it was copied from code by Teodoro.
There was a problem hiding this comment.
In general, should one claim authorship of a routine even if it was only slightly modified from a previous one,...
It depends ;-)
For simple boilerplate code like this, you can safely put your own name to let others know that you're the Allegro-expert. For more complex code you can share the credit via the \history tag.
Generally, we try to give more visibility to recent work.
src/pair_potential_types.F
Outdated
| allegro_dest%unit_energy = allegro_source%unit_energy | ||
| allegro_dest%unit_energy_val = allegro_source%unit_energy_val | ||
| allegro_dest%unit_cell = allegro_source%unit_cell | ||
| allegro_dest%unit_cell_val = allegro_source%unit_cell_val |
There was a problem hiding this comment.
It could be simpler to just write allegro_dest = allegro_source
src/pair_potential_types.F
Outdated
| allegro%unit_coords = 'NULL' | ||
| allegro%unit_forces = 'NULL' | ||
| allegro%unit_energy = 'NULL' | ||
| allegro%unit_cell = 'NULL' |
There was a problem hiding this comment.
You can use allegro = allegro_pot_type() instead after you have defined proper defaults at the definition of this type.
| IF (ASSOCIATED(allegro)) THEN | ||
| DEALLOCATE (allegro) | ||
| END IF | ||
| END SUBROUTINE pair_potential_allegro_release |
There was a problem hiding this comment.
Do you really need this function (especially if you have removed the create routine)?
There was a problem hiding this comment.
As for the create routine, I would like to keep it for consistency with previous code.
src/manybody_allegro.F
Outdated
| IF (use_virial) THEN | ||
| virial(1, 1) = 0.0_dp; virial(1, 2) = 0.0_dp; virial(1, 3) = 0.0_dp | ||
| virial(2, 1) = 0.0_dp; virial(2, 2) = 0.0_dp; virial(2, 3) = 0.0_dp | ||
| virial(3, 1) = 0.0_dp; virial(3, 2) = 0.0_dp; virial(3, 3) = 0.0_dp |
There was a problem hiding this comment.
I am aware that this branch is not yet properly implemented. But just write virial = 0.0_dp and below pv_nonbond = pv_nonbond + virial
simplified pair pot types and delete leftover write now prettified revert back to pointer in allegro reduced number of lines for virial replace pointers with allocatables where possible prettified again
3 participants
Allegro is an equivariant NN interatomic potential for large-scale systems.