Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update QTSH #212

Merged
merged 14 commits into from
Jun 19, 2024
Merged

Update QTSH #212

merged 14 commits into from
Jun 19, 2024

Conversation

DaehoHan
Copy link
Contributor

@DaehoHan DaehoHan commented Jun 7, 2024

No description provided.

@@ -100,6 +100,9 @@ void export_dyn_control_params_objects(){
.def_readwrite("fssh3_max_steps", &dyn_control_params::fssh3_max_steps)
.def_readwrite("fssh3_err_tol", &dyn_control_params::fssh3_err_tol)

///================= FSSH3 specific ====================
Copy link
Member

Choose a reason for hiding this comment

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

should be "QTSH specific"

double sum = 0.0;
for(int n=0; n<nadi; n++){
for(int k=0; k<n; k++){
sum += ham.children[itraj]->dc1_adi[idof]->get(k, n).real() * dyn_var.dm_adi[itraj]->get(k, n).imag();
//sum += ham.children[itraj]->dc1_adi[idof]->get(k, n).real() * dyn_var.dm_adi[itraj]->get(k, n).imag();
Copy link
Member

Choose a reason for hiding this comment

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

i think, the version without T matrix should still work, but the call of the function should be after the electronic integration and the density matrix update call (so that the density matrix is consistent with the new derivative couplings)

@@ -39,6 +39,7 @@ void update_Hamiltonian_variables(dyn_control_params& prms, dyn_variables& dyn_v
- 0: in response to changed q
- 1: in response to changed p
- 2: in response to changed electronic amplitudes
- 3: in response to changed p_k in QTSH
Copy link
Member

Choose a reason for hiding this comment

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

let's not complicate this function by such a pretty-methodology-specific case; this update would be of the kind "0" and "1" - because 0 - is in response to changed NAC (that depends on q) and 1 - is in response to changed p (which is changed indirectly to since p_k changes).
I'd still think on p as central variables - the kinematic p is only needed as an auxiliary variable.

@@ -456,6 +456,14 @@ class dyn_control_params{
double fssh3_err_tol;


//=========== QTSH options ==========
/**
Whether to use QTSH
Copy link
Member

Choose a reason for hiding this comment

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

add options possible and what they mean: 0 - not using QTSH; 1 - using QTSH - explain to user what is actually changed when we use QTSH (a different convention for the force to use)

@@ -1562,8 +1573,10 @@ void compute_dynamics(dyn_variables& dyn_var, bp::dict dyn_params,
if(prms.thermally_corrected_nbra==1 && prms.tcnbra_do_nac_scaling==1){ remove_thermal_correction(dyn_var, ham, prms); }

// Update vib Hamiltonian to reflect the change of the momentum
update_Hamiltonian_variables(prms, dyn_var, ham, ham_aux, py_funct, params, 1);

if(prms.use_qtsh==0){
Copy link
Member

Choose a reason for hiding this comment

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

i don't agree with this conditional call (nested inside if block) of this function - basically, is should be always called but would do nothing for QTSH. You are right - this could be more efficient your way, but adds the complexity of the code. In this case I'd sacrifice the efficiency for the sake of clarity and simplicity

*dyn_vars.f = ham.forces_adi(effective_states).real();
}// rep_force == 1

// Non-classical term calculations
Copy link
Member

Choose a reason for hiding this comment

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

the term "non-classical" is confusing here - do you mean the total QTSH force, or only the Ehrenfest-like correction to the adiabatic forces? I think we could just use the regular forces variable to keep the entire QTSH force, if this method is used.

@alexvakimov alexvakimov merged commit 1578ac9 into Quantum-Dynamics-Hub:devel Jun 19, 2024
@DaehoHan DaehoHan deleted the update_QTSH_v2 branch August 5, 2024 16:38
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.

2 participants