Skip to content

623 enhance safety#624

Open
johanneshofmann87 wants to merge 10 commits intomasterfrom
623-enhance-safety
Open

623 enhance safety#624
johanneshofmann87 wants to merge 10 commits intomasterfrom
623-enhance-safety

Conversation

@johanneshofmann87
Copy link
Copy Markdown
Contributor

@johanneshofmann87 johanneshofmann87 commented Mar 31, 2026

Adds a centralized safety/validation layer to ensure Hamiltonian-global variables and core arrays are initialized and correctly set by each Ham_Set, preventing undefined/uninitialized state from propagating into the Monte Carlo core.

Changes Made

  • Initialize key Hamiltonian globals (Ndim, N_FL, N_SUN, Ltrot, Thtrot, Projector, Symm) to sentinel/safe defaults in Alloc_Ham.
  • Validate required Hamiltonian globals and allocations right after ham%Ham_set() (including Op_V, Op_T, and projector trial wavefunctions).
  • Ensure Thtrot is explicitly initialized in relevant Ham_Set implementations.
  • Add Hermiticity check in Op_set to validate operator matrices, merging Hermiticity and diagonal-detection into a single O(N²) upper-triangle pass.
  • Add Op_equal function in Operator_mod to compare two Operator instances on their defining properties (used for structural checks).
  • Add Op_V_is_symmetric function in Operator_mod that verifies the product (I+g₁V₁)···(I+gₙVₙ) equals its reverse — satisfied by palindromic arrangement or mutually commuting operators.
  • Validate that all projector indices P(i) in Op_V and Op_T are within [1, Ndim] in Validate_Ham_Variables, preventing out-of-bounds memory access before any matrix indexing occurs.
  • Validate that the Op_V operator sequence is symmetric when Symm = .true., terminating with a clear error if not.
  • Add iso_fortran_env import for error_unit in Operator_mod.
  • Add test suite coverage for Op_set validation (tests 28–32), including Hermiticity failure, type failure, projector index failure, and 1×1 matrix failure.
  • Add test suite coverage for Op_equal and Op_V_is_symmetric (tests 33–35): operator equality comparison, symmetric Op_V pass, and asymmetric Op_V fail.

@johanneshofmann87 johanneshofmann87 linked an issue Mar 31, 2026 that may be closed by this pull request
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

Adds a centralized safety/validation layer to ensure Hamiltonian-global variables and core arrays are initialized and correctly set by each Ham_Set, preventing undefined/uninitialized state from propagating into the Monte Carlo core.

Changes:

  • Initialize key Hamiltonian globals (Ndim, N_FL, N_SUN, Ltrot, Thtrot, Projector, Symm) to sentinel/safe defaults in Alloc_Ham.
  • Validate required Hamiltonian globals and allocations right after ham%Ham_set() (including Op_V, Op_T, and projector trial wavefunctions).
  • Ensure Thtrot is explicitly initialized in several Hamiltonian Ham_Set implementations.

Reviewed changes

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

Show a summary per file
File Description
Prog/main.F90 Calls Validate_Ham_Variables() immediately after ham%Ham_set(); removes redundant projector WF allocation pre-check now covered by validation.
Prog/Hamiltonian_main_mod.F90 Adds sentinel initialization (Init_Ham_Variables) and post-Ham_Set validation (Validate_Ham_Variables); exports the validator.
Prog/Hamiltonians/Hamiltonian_Hubbard_smod.F90 Explicitly initializes Thtrot = 0 before computing Ltrot.
Prog/Hamiltonians/Hamiltonian_Kondo_smod.F90 Explicitly initializes Thtrot = 0 before computing Ltrot.
Prog/Hamiltonians/Hamiltonian_Spin_Peierls_smod.F90 Explicitly initializes Thtrot = 0 before computing Ltrot.

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

@johanneshofmann87
Copy link
Copy Markdown
Contributor Author

Thanks, @jonasschwab , for already taking a look at this. We run into this today where we noticed that we ran ALF with N_SUN=0, as it was not set explicitly anywhere. surprisingly, the code ran rather well and the results (mostly) made sense. but the fermion determinate did NOT influence the acceptance/rejection of the Metropolis step.

Since we are at enhancing the 'security' of the model definition: Are you aware of other implicit assumptions that we are relying on without checking them explicitly?
The hermitian matrix check (and tests in the test suite) are added now.

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 14 out of 14 changed files in this pull request and generated 3 comments.


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

Comment thread Prog/Operator_mod.F90
Comment thread Prog/Operator_mod.F90 Outdated
Comment thread testsuite/Prog.tests/CMakeLists.txt Outdated
@johanneshofmann87
Copy link
Copy Markdown
Contributor Author

@copilot During review on GitHub, copilot provided the following suggestion:
"Op_set now performs an explicit O(N^2) Hermiticity scan over the upper triangle, and later in the routine it scans the upper triangle again to determine whether the operator is diagonal. For large-N diagonal operators this duplicates work in the setup path. Consider merging the Hermiticity and diagonal-detection logic into a single pass over (i,j) to avoid the second traversal and keep the validation cost minimal."

Agent-Logs-Url: https://github.com/ALF-QMC/ALF/sessions/3f0f3c08-432d-4df4-bd80-a092b67bc03d

Co-authored-by: johanneshofmann87 <129625831+johanneshofmann87@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 1, 2026

@copilot During review on GitHub, copilot provided the following suggestion:
"Op_set now performs an explicit O(N^2) Hermiticity scan over the upper triangle, and later in the routine it scans the upp...

Done in commit b952e94. The Hermiticity scan and diagonal-detection now share a single O(N²) upper-triangle pass: Op%diag is initialized to .true. before the loop, and inside each iteration we first validate Hermiticity, then (guarded by if (Op%diag)) check whether the upper-triangle element is non-zero — only a single check per element needed since a passed Hermiticity check implies Op%O(I,J)==0 ↔ Op%O(J,I)==0. The redundant second loop is removed.

@johanneshofmann87
Copy link
Copy Markdown
Contributor Author

@jonasschwab @fassaad I think this update covers some important consistency checks for models implemented by users that I am aware of. Do you know of additional implicit assumptions that we are not enforcing in the code?

@jonasschwab
Copy link
Copy Markdown
Member

One could also add a test if the arrangement of operators is indeed symmetric if Symm is true.

- Introduced Op_equal function to compare Operator instances.
- Added Op_V_is_symmetric function to verify symmetry of operator sequences.
- Implemented tests for Op_equal and Op_V_is_symmetric, including cases for both symmetric and asymmetric arrangements.
- Updated CMakeLists.txt to include new test files.
Copy link
Copy Markdown
Contributor Author

@johanneshofmann87 johanneshofmann87 left a comment

Choose a reason for hiding this comment

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

One could also add a test if the arrangement of operators is indeed symmetric if Symm is true.

I implemented such a test in the last commit.

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 17 out of 17 changed files in this pull request and generated 3 comments.


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

Comment thread Prog/Operator_mod.F90
Comment thread Prog/Hamiltonian_main_mod.F90
Comment thread Prog/Operator_mod.F90
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ALF-QMC/ALF/sessions/17c4f4b1-6a95-4987-9d7b-163442ea8aeb

Co-authored-by: johanneshofmann87 <129625831+johanneshofmann87@users.noreply.github.com>
@johanneshofmann87
Copy link
Copy Markdown
Contributor Author

@copilot update the description of this PR to reflect the actual changes implemented here.

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 7, 2026

@copilot update the description of this PR to reflect the actual changes implemented here.

Updated the PR description to accurately list all changes implemented: sentinel initialization, Validate_Ham_Variables checks (globals, allocations, projector index bounds, Symm symmetry), the Op_equal and Op_V_is_symmetric functions, merged Hermiticity+diagonal detection, error_unit import fix, and the full test coverage (tests 28–35).

@johanneshofmann87 johanneshofmann87 marked this pull request as ready for review April 7, 2026 13:40
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.

Enhance safety

4 participants