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

Writing adjacency to output #497

Merged
merged 3 commits into from
Jun 29, 2021
Merged

Writing adjacency to output #497

merged 3 commits into from
Jun 29, 2021

Conversation

hoelzerC
Copy link
Contributor

@hoelzerC hoelzerC commented Jun 29, 2021

Optional output of adjacency and coordination number for GFN-FF

Signed-off-by: C.Hoelzer [email protected]

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

The neighbour/adjacency list is not really an output of the calculation, therefore I don't think it makes sense to write it with the singlepoint calculation. Preferably, we can generate such output with the topology generation or extract it from the topology restart file directly.

One possibility would be to create such output in the topology subcommand when generating a topology restart file ahead of an actual singlepoint calculation:
https://github.com/grimme-lab/xtb/blob/master/src/prog/topology.f90

@hoelzerC
Copy link
Contributor Author

The method was refactored into the gfnff_setup routine. Now, the adjacency list is written with the topo restart file (write_restart_gff).

@hoelzerC hoelzerC requested a review from awvwgk June 29, 2021 14:31
write(ifile, *) '# indices of neighbouring atoms (max seven); connectivity given by last entry;'
do i = 1, size(topo%nb,2) ! #atoms
do j = 1, size(topo%nb,1) ! 20
write(ifile, '(I2,X)', advance='no') topo%nb(j,i)
Copy link
Member

@awvwgk awvwgk Jun 29, 2021

Choose a reason for hiding this comment

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

This statement has several issues, first since you are using a non-advancing output it will produce a trailing whitespace from the format specifier. Second, leaving away the width of the x identifier is a standard extension and we want to avoid relying on those.

Third, the integer field will overflow for more than 99 atoms:

❯ tail gfnff_adjacency
** ** **  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  3  
** ** **  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  3  
** ** **  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  3  
** ** **  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  3  
**  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  
**  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  
**  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  
**  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  
**  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  
86  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1

Finally, printing the padded adjacency list exposes an implementation detail like the preallocated number of entries for each atom, which is fragile in case we change this internally. Maybe a more practical format might be

do i = 1, size(topo%nb, 2)
  write(ifile, '(*(i0:, 1x))') (topo%nb(j, i), j = 1, topo%nb(size(topo%nb, 1), i))
end do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I adapted the output format.

@hoelzerC hoelzerC merged commit aecd28a into master Jun 29, 2021
@hoelzerC hoelzerC deleted the gfnff_adjacency branch June 29, 2021 20:10
@awvwgk awvwgk added this to the v6.5.0 milestone Apr 13, 2022
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.

2 participants