Skip to content

Resolve "implement Hamiltonian variables check"#566

Merged
fassaad merged 27 commits into
masterfrom
316-implement-hamiltonian-variables-check
Mar 25, 2026
Merged

Resolve "implement Hamiltonian variables check"#566
fassaad merged 27 commits into
masterfrom
316-implement-hamiltonian-variables-check

Conversation

@ALF-Import-Bot

Copy link
Copy Markdown

In GitLab by @lidiastocker on Jul 23, 2025, 08:53 UTC:

Closes #316
Closes #276

Assignees: @lidiastocker

Reviewers: @jonasschwab

Approved by: @jonasschwab

Migrated from GitLab: https://git.physik.uni-wuerzburg.de/ALF/ALF/-/merge_requests/236

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Jul 23, 2025, 08:53 UTC:

requested review from @johanneshofmann87

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Aug 4, 2025, 12:25 UTC:

added 3 commits

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Aug 4, 2025, 13:24 UTC:

added 1 commit

  • 6c31326 - runtime variables on separatedfile

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Aug 4, 2025, 13:33 UTC:

added 1 commit

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Aug 4, 2025, 13:50 UTC:

I created a separate file QMC_runtime_var, which is used by main and can also be accessed by Hamiltonian_##NAME##_smod (or any other file) to read QMC variables.

As of now, if, for example, a Hamiltonian does not support the sequential update scheme, yet "Sequential": true is set (using pyalf notation), one should add the following lines to Hamiltonian_##NAME##_smod.F90:

Subroutine Ham_Set
    Use QMC_runtime_var

    ...

    if (Sequential) then
        Write(error_unit,*) 'Sequential update scheme not applicable to the Hamiltonian_##NAME##'
        CALL Terminate_on_error(ERROR_GENERIC, __FILE__, __LINE__)
    endif

    ...
end Subroutine Ham_Set

This is not the most elegant solution, as it raises an error only when calling .run(), but I do not think we can overcome this (as the filename suggest, we are dealing with runtime variables and, even if in this case one could stop the code at compile time already, this would imply a mix of the two types).

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Aug 4, 2025, 14:00 UTC:

added 1 commit

  • dae2aca - Revert "rename MC_var -> QMC_var"

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @lidiastocker on Aug 4, 2025, 14:08 UTC:

added 1 commit

  • 1b873a2 - rename MC_runtime_var -> QMC_runtime_var

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @jonasschwab on Aug 5, 2025, 08:59 UTC:

I would move the reading and MPI broadcast of the variables to the module. This will clean up main.F90 a bit.

For added cleanliness, one could even make most of the variables private and access them via getters, e.g. functions like get_sequential().

@ALF-Import-Bot

Copy link
Copy Markdown
Author

Discussion in GitLab:

In GitLab by @jonasschwab on Aug 5, 2025, 09:08 UTC:

Should we move all these variable in there? Giving the Hamiltonian access to so many variables opens avenues for breaking backwards compatibility and additional ways for user to shoot themselves in the foot. Especially things like Stab_nt and udvst are probably better kept for internal use.

There are also a few things that don't belong in such a big scope, like ierr and toggle. I suppose they're mainly there because of the early state of this draft.

In GitLab by @johanneshofmann87 on Aug 5, 2025, 09:28 UTC:

Yes, this is a very early stage of the draft.

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @johanneshofmann87 on Aug 5, 2025, 09:26 UTC:

@jonasschwab and @lidiastocker ,
We can probably be a little bit more selective and focus on actual QMC setting variables. I wouldn't mind to make some variables private, but one of the ideas is that we can get access to them from within the model. In some special implementations, for example, sequential updates are no longer allowed and should be disabled. In the past, I was then just not allocating some Operator members or used other "hacks" to trigger segmentation faults. While this worked for me and I could recall what was going on, this is a pretty dirty workaround and not maintainable in the long run.
So we would have to provide "get" and "set" routines.
@lidiastocker : I'll mark some of the variables as comments in the source code changes section.

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @jonasschwab on Sep 23, 2025, 14:42 UTC:

approved this merge request

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @johanneshofmann87 on Sep 29, 2025, 12:52 UTC:

resolved all threads

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @fassaad on Sep 29, 2025, 13:01 UTC:

This is of course much cleaner. Have we really checked all the environment variables?

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @fassaad on Oct 28, 2025, 15:56 UTC:

Put a lock on the variables. You can only change the variable once. Setters and getters. Variables have to be private, and write routines to update them. These routines can issue warnings!

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @jonasschwab on Oct 28, 2025, 18:33 UTC:

added 1 commit

  • db8a6242 - runtime_error_mod: Use setters and getters

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @jonasschwab on Oct 28, 2025, 18:41 UTC:

added 1 commit

  • 0344a07 - QMC_runtime_var: Use setters and getters

Compare with previous version

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @jonasschwab on Oct 28, 2025, 18:42 UTC:

I took the liberty to add setters and getters (with a little help from ChatGPT).

We could now add something like a bool allow_override_var.

@ALF-Import-Bot

Copy link
Copy Markdown
Author

In GitLab by @jonasschwab on Oct 28, 2025, 18:45 UTC:

added 76 commits

  • 0344a07...a5e4c2f - 75 commits from branch master
  • d5b856b - Merge branch 'master' into 316-implement-hamiltonian-variables-check

Compare with previous version

@johanneshofmann87

Copy link
Copy Markdown
Contributor

@lidiastocker @jonasschwab @fassaad I have added a lock which blocks any change to the QMC runtime variables once the initialisation is completed. Thanks to Jonas' work on the get/set functions, this was straight forward. I also merged the current master to resolve any merge conflicts. This should be ready now, as far as I'm concerned.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request implements a dedicated module for QMC (Quantum Monte Carlo) runtime variables, addressing issues #316 and #276. The PR enables Hamiltonian files to access QMC settings by refactoring all QMC-related variables from main.F90 into a new QMC_runtime_var_mod.F90 module. The refactoring introduces a getter/setter pattern with a locking mechanism to prevent modifications after initialization, ensuring configuration consistency during runtime.

Changes:

  • Created new QMC_runtime_var_mod.F90 module with getter/setter accessors for all QMC runtime variables, including support for conditional compilation (MPI, TEMPERING)
  • Refactored main.F90 to use the new module via getter/setter functions instead of direct variable access
  • Updated Makefile to include the new module in the build process

Reviewed changes

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

File Description
Prog/QMC_runtime_var_mod.F90 New module that encapsulates all QMC runtime variables with getter/setter accessors, locking mechanism, and read/broadcast functions for MPI environments
Prog/main.F90 Removed QMC variable declarations and updated all references to use getters/setters from QMC_runtime_var module; reorganized imports for better clarity
Prog/Makefile Added QMC_runtime_var_mod.o to OBJS and QMC_runtime_var_mod.mod to MODS for build dependencies

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

Comment thread Prog/main.F90 Outdated
Comment thread Prog/QMC_runtime_var_mod.F90 Outdated
Comment thread Prog/QMC_runtime_var_mod.F90 Outdated
Comment thread Prog/QMC_runtime_var_mod.F90 Outdated
Comment thread Prog/QMC_runtime_var_mod.F90
Comment thread Prog/QMC_runtime_var_mod.F90
@johanneshofmann87

Copy link
Copy Markdown
Contributor

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot AI commented Feb 15, 2026

Copy link
Copy Markdown
Contributor

@johanneshofmann87 I've opened a new pull request, #600, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 4 commits February 15, 2026 18:36
… spelling and indentation

Co-authored-by: johanneshofmann87 <129625831+johanneshofmann87@users.noreply.github.com>
Fix code review issues: missing imports, duplicate lock, and MPI file I/O
@fassaad

fassaad commented Mar 25, 2026

Copy link
Copy Markdown
Member

I did not check everything, and it looks very nice. One question though, there is a lot of code, e.g. set and get, has this been coded with AI? In any case, I think that we should merge this before merging #592 . This pull request introduces more variables for MALA updates.

@fassaad fassaad merged commit b4d386b into master Mar 25, 2026
25 checks passed
@fassaad fassaad deleted the 316-implement-hamiltonian-variables-check branch March 25, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

implement Hamiltonian variables check QMC settings in dedicated module

7 participants