Skip to content

Enhancement: Report projected gradient norm during MBIS optimization#38

Open
Ao-chuba wants to merge 4 commits intotheochem:mainfrom
Ao-chuba:feature/projected-gradient
Open

Enhancement: Report projected gradient norm during MBIS optimization#38
Ao-chuba wants to merge 4 commits intotheochem:mainfrom
Ao-chuba:feature/projected-gradient

Conversation

@Ao-chuba
Copy link
Copy Markdown

@Ao-chuba Ao-chuba commented Jan 30, 2026

The generic optimize_pro_model previously reported the standard Euclidean norm of the gradient. However, MBIS optimization serves as a constrained problem (non negative populations and exponents). When parameters are at their bounds, the raw gradient may be non zero (pointing out of the feasible region) even at the solution. This made the reported "grad.rms" a potentially misleading metric for checking convergence.
I have modified src/denspart/vh.py to correctly calculate and report the projected gradient norm:

  • Located the parameter bounds so they are accessible within the optimization callback.
  • Added logic to mask (zero out) gradient components that act against an active boundary constraint.
  • Updated the log output to print the norm of this grad_proj instead of the raw gradient.

Verified using the standard horton3 water molecule example. The optimization converged successfully, and the reported grad.rms now consistently reports the true distance to the constrained solution, dropping to ~1e-9 at convergence

Screenshot 2026-01-30 200859

closes #37

@Ao-chuba
Copy link
Copy Markdown
Author

Ao-chuba commented Feb 7, 2026

Hi @tovrstra ,
The checks failing on Python 3.11 are due to a dependency issue, unrelated to the changes in this PR. The error ImportError: cannot import name 'sph_harm' from 'scipy.special' originates from the grid package.
Cause:

  • scipy 1.15.0 (released recently) removed scipy.special.sph_harm.
  • The grid package currently relies on this removed function.
  • The CI environment is installing the latest scipy, causing grid to break.

To fix this i have I have pinned scipy < 1.15 in pyproject.toml to ensure compatibility with grid and Since denspart tests were using the new sph_harm_y (available only in scipy >= 1.15), pinning the older version caused local test failures. I added a backward-compatibility wrapper in tests/test_properties.py to map sph_harm_y calls back to sph_harm when the newer function is missing.

Copy link
Copy Markdown

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

Updates MBIS optimization progress reporting so the logged grad.rms reflects the projected gradient norm under bound constraints, making convergence diagnostics meaningful when parameters sit on active bounds.

Changes:

  • Exposes parameter bounds inside the trust-constr callback and computes a masked (“projected”) gradient for reporting.
  • Adjusts spherical-harmonics test imports to work across SciPy variants (sph_harm_y vs sph_harm).
  • Pins SciPy to <1.15 in pyproject.toml.

Reviewed changes

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

File Description
src/denspart/vh.py Computes and reports projected gradient norm in the optimization callback.
tests/test_properties.py Adds SciPy import fallback for spherical harmonics used in tests.
pyproject.toml Introduces an upper bound on the SciPy dependency version.

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

@Ao-chuba Ao-chuba force-pushed the feature/projected-gradient branch from 3e92f5e to f900082 Compare February 9, 2026 12:29
@Ao-chuba Ao-chuba force-pushed the feature/projected-gradient branch from 92f8cca to 9f91dd6 Compare February 9, 2026 12:34
@Ao-chuba
Copy link
Copy Markdown
Author

Ao-chuba commented Feb 9, 2026

@tovrstra @PaulWAyers
To clarify the timeline of changes in this PR:

  • Initial Attempt: My first commit implemented the projected gradient reporting, but the tests failed due to a scipy import error originating in the grid dependency.
  • Intermediate Fix: I temporarily pinned scipy<1.15 and added a compatibility wrapper to bypass that dependency issue.
  • Current State: Since the upstream fix in grid (PR #291) has now been merged, the scipy import error is resolved at the source.
    1. I have reverted the temporary scipy pinning and compatibility wrappers, as they are no longer needed.
    2. I have also refined the optimize_pro_model function in src/denspart/vh.py . Specifically, I improved the masking logic for the projected gradient to use robust, scale-aware tolerance checks for the upper and lower bounds, addressing the recent code review feedback (co pilot)

@Ao-chuba
Copy link
Copy Markdown
Author

@tovrstra, I’ve updated the PR. When you have some time, could you please review it? I’d really appreciate your feedback. Thank you

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.

Enhancement: Report projected gradient norm during MBIS optimization

2 participants