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

Fix exit condition in calc_distances #1168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

foxtran
Copy link
Contributor

@foxtran foxtran commented Feb 1, 2025

Probably affects #621

Before patch, one distance were skipped between atoms # N and # N-1. For single-bonded systems (like H2) it is critical since it means that they were not bounded and then a lot of artefacts happen.

@TyBalduf
Copy link
Contributor

TyBalduf commented Feb 1, 2025

Might be missing something, but I don't see how this would affect #621. This function seems to only be used for printing a summary of atom distances at the end of a calculation, but the NaN's in the linked issue start to occur during the initial SCF.

The PR here does fix a real bug, as the outputs from the linked issue do show that the "selected distances" section isn't being printed, which should only occur if no distances were found. More generally, the summary should have been printing N(N-1)/2 distances, but it presumably has always printed one less distance.

The code change looks reasonable to me, but I don't think it resolves #621.

@foxtran
Copy link
Contributor Author

foxtran commented Feb 1, 2025

@TyBalduf, yep. I found more bugs with H2 system, so it may not fix issue with #621

UPD: No, I fixed exactly this bug :( Just started collecting patches from scratch.

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