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

Refactor: change R_index in class AtomPair to type ModuleBase::vector3<int> #4243

Merged
merged 17 commits into from
Jun 13, 2024
Merged

Conversation

DylanWRh
Copy link

@DylanWRh DylanWRh commented May 27, 2024

Reminder

  • Have you explained the changes of codes in core modules of ESolver, HSolver, ElecState, Hamilt, Operator or Psi? (ignore if not applicable)

What's changed?

This pull request addresses the type inconsistency of the R_index variable across different modules. Previously, the R_index variable in the AtomPair class was of type std::vector<int>, while in some other modules it was of type ModuleBase::Vector3<int>. This inconsistency could potentially lead to confusion and errors. So I modify the code under AtomPair and also other modules which use its R_index for code consistency and maintenance.

  • Type of AtomPair::R_index is changed from std::vector<int> to std::vector<ModuleBase::Vector3<int>>.
  • Type of HContainer::tmp_R_index is changed from std::vector<int> to std::vector<ModuleBase::Vector3<int>>.
  • Return value of function AtomPair::get_R_index is changed from int* to ModuleBase::Vector3<int>.
  • Indexing of R_index is changed from R_index[i*3], R_index[i*3+1], R_index[i*3+2] to R_index[i].x, R_index[i].y, R_index[i].z. The functions of size and memory calculation is also modified. (i.e. remove some *3 and /3 operations).
  • Other modules that calls the function HContainer::tmp_R_index is changed to fit the new return type. For example, line 87 and 88 in module_io/td_current_io.cpp is changed from
    const int* r_index = tmp_ap.get_R_index(ir);
    hamilt::BaseMatrix<double>* tmp_matrix = tmp_ap.find_matrix(r_index[0], r_index[1], r_index[2]);
    to
    const ModuleBase::Vector3<int> r_index = tmp_ap.get_R_index(ir);
    hamilt::BaseMatrix<double>* tmp_matrix = tmp_ap.find_matrix(r_index.x, r_index.y, r_index.z);

Important Notes

  • When fail to find R_index, the original AtomPair::get_R_index will return nullptr while the modified version will return ModuleBase::Vector3<int>(-1, -1, -1))
  • TESTING code in test_hcontainer.cpp and test_hcontainer_complex.cpp is changed to fit the new return type.
  • Though the checks are passed, I still cannot guarantee that the related code is thoroughly and properly modified. So it really need careful reviewing before merging into the main branch.

@DylanWRh DylanWRh changed the title Refactor R_index in AtomPair Refactor: change R_index in class AtomPair to ModuleBase::vector3 May 27, 2024
@DylanWRh DylanWRh changed the title Refactor: change R_index in class AtomPair to ModuleBase::vector3 Refactor: change R_index in class AtomPair to type ModuleBase::vector3<int> May 27, 2024
@DylanWRh DylanWRh marked this pull request as ready for review May 27, 2024 12:16
@DylanWRh DylanWRh requested a review from dyzheng June 5, 2024 12:05
@dyzheng dyzheng merged commit 156dac2 into deepmodeling:develop Jun 13, 2024
13 checks passed
@dyzheng dyzheng mentioned this pull request Jun 19, 2024
16 tasks
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