Skip to content

Failproof infinity handling #409

Open
kshitij-05 wants to merge 5 commits intomasterfrom
kshitij/fix/cmath_infinity
Open

Failproof infinity handling #409
kshitij-05 wants to merge 5 commits intomasterfrom
kshitij/fix/cmath_infinity

Conversation

@kshitij-05
Copy link
Copy Markdown
Collaborator

@kshitij-05 kshitij-05 commented Mar 25, 2026

Description:

Replaces fragile use of raw INFINITY / std::numeric_limits<double>::infinity() in GaussianPotentialPrimitive exponents with a named constant and input validation.

  • Introduce libint2::infinite_exponent as the canonical sentinel for point-charge exponents
  • Add validate_gaussian_potential_primitive()— throws on NaN exponent, negative exponent, or NaN coefficient
  • Add debug assertions in q_gau_gm_eval::operator() to catch invalid primitives
  • Validate omega, lambda, sigma parameters in make_q_gau_data_{erf,erfc,erfx} factories
  • Validate SAP basis data after read_sap_basis_library() (debug mode)
  • Add unit tests for all validation paths

No API breakage — GaussianPotentialPrimitive remains an aggregate; {libint2::infinite_exponent, 1.0} is a drop-in replacement for {INFINITY, 1.0}.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to make generalized Gaussian nuclear potential (q_gau) handling more robust and clearer by standardizing the “point-charge” exponent sentinel and adding input validation/diagnostics.

Changes:

  • Introduces libint2::infinite_exponent as the canonical sentinel for point-charge (1/r) primitives and updates documentation/examples to use it.
  • Adds validation helpers/guards for q_gau inputs (new validate_gaussian_potential_primitive, debug-time asserts in the core evaluator, and runtime parameter checks in make_q_gau_data_* factories).
  • Adds unit tests covering the new validation behavior and factory argument rejection.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
include/libint2/shell.h Adds infinite_exponent sentinel and a new primitive validation helper.
include/libint2/boys.h Adds debug assertions validating q_gau primitive inputs before evaluation.
include/libint2/basis.h.in Switches to the sentinel in factories and adds argument validation (throws/asserts).
export/tests/unit/test-1body.cc Adds unit tests for primitive and factory validation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Owner

@evaleev evaleev left a comment

Choose a reason for hiding this comment

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

Review

Clean, well-scoped PR — the named constant and input validation are clear improvements. A few suggestions:

1. validate_gaussian_potential_primitive is declared but never called in production code

The factories in basis.h.in validate their own parameters (omega, lambda, sigma) but don't validate the GaussianPotentialPrimitive objects they construct. User-constructed primitives go unvalidated unless the user explicitly calls the function. Consider calling it inside q_gau_gm_eval::operator() (non-debug), or at minimum documenting that manual constructors of GaussianPotentialData should call it.

2. Debug assertions in boys.h duplicate the loop

The debug assertion block (lines 2199–2210) iterates over primitives and then the existing code immediately iterates again. Consider merging the checks into the existing loop body to keep it DRY:

for (const auto& prim : primitives) {
  assert(!isnan(prim.exponent) && "...");
  assert(prim.exponent >= 0.0 && "...");
  assert(isfinite(prim.coefficient) && "...");

  if (isinf(prim.exponent)) { ... }
  // ...
}

3. SAP validation should be unconditional

The SAP basis data is read from disk — a system boundary where validation should not be debug-only. A corrupt .g94 file would silently pass in release builds. Consider using validate_gaussian_potential_primitive() with a runtime throw instead of assert.

4. Missing explicit <cmath> include in shell.h

validate_gaussian_potential_primitive uses std::isnan/std::isfinite but shell.h doesn't explicitly include <cmath>. Works transitively today but fragile for a standalone validation function in a public header.

5. Minor: make_q_gau_data_erfx — should lambda/sigma allow zero?

omega is rejected if <= 0, but lambda and sigma only require isfinite. If zero is not physically meaningful for those, tighten the validation; otherwise a brief comment would help.

6. Minor: consider constexpr on validate_gaussian_potential_primitive

Would enable compile-time validation of literal-constructed primitives in the future.

Overall: solid work, well-tested. Main actionable items are #3 (SAP validation unconditional) and #4 (<cmath> include).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +852 to +861
for (size_t z = 0; z < sap_by_Z.size(); ++z) {
for (const auto& p : sap_by_Z[z]) {
if (!std::isfinite(p.exponent) || p.exponent <= 0.0)
throw std::invalid_argument(
"make_q_gau_data: SAP basis contains invalid exponent");
if (!std::isfinite(p.coefficient))
throw std::invalid_argument(
"make_q_gau_data: SAP basis contains non-finite coefficient");
}
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The PR description says SAP basis validation should be debug-only, but this loop throws std::invalid_argument in all builds. If runtime validation is intended only for debug, guard this with #ifndef NDEBUG (or convert to assertions); otherwise please update the PR description to reflect the release-behavior change and consider the user-facing impact of new exceptions from make_q_gau_data(..., sap_basis_name).

Copilot uses AI. Check for mistakes.
/// Build GaussianPotentialCentersData for erfx(ω,λ,σ)/r model.
/// Computes (λ·erf(ωr) + σ·erfc(ωr))/r = σ/r - (σ-λ)·erf(ωr)/r
/// Computes (λ·erf(ωr) + σ·erfc(ωr))/r = σ/r - (σ-λ)·erf(ωr)/r.
/// Zero is valid for lambda (pure erfc) and sigma (pure -erf).
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The comment says “Zero is valid for ... sigma (pure -erf)”, but with sigma=0 the expression reduces to (lambda * erf(ωr))/r (and equals -erf/r only for a specific lambda, e.g., lambda=-1). Please adjust this wording to avoid documenting an incorrect special case.

Suggested change
/// Zero is valid for lambda (pure erfc) and sigma (pure -erf).
/// Zero is valid for lambda and sigma: λ = 0 gives pure erfc (if σ ≠ 0),
/// while σ = 0 gives an erf-only term scaled by λ.

Copilot uses AI. Check for mistakes.
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