Skip to content

Enable two-center case in test_poisson_bvp_gives_the_correct_laplacian - #331

Open
Phantom8420 wants to merge 1 commit into
theochem:masterfrom
Phantom8420:fix/enable-two-center-poisson-laplacian-test
Open

Phantom8420 wants to merge 1 commit into
theochem:masterfrom
Phantom8420:fix/enable-two-center-poisson-laplacian-test

Conversation

@Phantom8420

Copy link
Copy Markdown

This case was commented out with # TODO: Couldn't get the following test to pass. I ran it against current master (two Gaussians at (0,0,0) and (1,1,1), default params) and it passes: max abs error ~6e-3 on grid points and ~3e-14 on random points, both under the existing atol=1e-2. Looks like it was fixed as a side effect of other work since this was written, just never re-checked. Uncommenting it.

this was commented out as failing. it passes now with default params (verified locally, atol=1e-2, both on-grid and random points).

@PaulWAyers PaulWAyers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like a good contribution to me. I'd want @marco-2023 and/or @Ao-chuba to confirm that this test now no longer needs to be commented out, however, as I'm not especially familiar with this part of the code.

@Phantom8420

Copy link
Copy Markdown
Author

Gentle bump. CI is green on all platforms here, including the case that was commented out. Happy to add anything that would help confirm it's safe to enable.

@Ao-chuba

Ao-chuba commented Sep 10, 2026

Copy link
Copy Markdown
Member

@Specter842
Based on my benchmarking work #315, the BVP solver has been improved since then. we confirmed this when testing the original water dimer from #215 and the BVP solver now handles it without crashing, so enabling this two-center Laplacian test is consistent with all of this. If CI is green, I think it is safe to merge.
also there is no logic change in the code so its a minimal change but correct

This branch has not been deployed

No deployments
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.

3 participants