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

Libmoinfo rework, part 2 #3155

Merged
merged 35 commits into from
May 24, 2024
Merged

Libmoinfo rework, part 2 #3155

merged 35 commits into from
May 24, 2024

Conversation

TiborGY
Copy link
Contributor

@TiborGY TiborGY commented Apr 14, 2024

Description

This is the continuation of my previous MOInfo PR #3112, with the following general themes:

  • Adding new docstrings and making existing ones machine readable for VS Code (and probably also Doxygen)
  • Rebalancing the class hierarchy by sinking things that get reused to the base class, and hoisting things up in the inheritance chain that only get used in one derived class
  • Reducing visible and mutable state, as well as the contact surface of the classes. This is achieved by making data and functions as private as possible, providing access to data through getters returning const& when necessary, and making a few data members const. Having const members in classes is sometimes considered to be bad form, but AFAIK all of the downsides are already incurred on account of MOInfoBase having reference members.

The git history on this branch is admittedly a bit messy, I changed my mind about a couple of things. LMK if that is a problem.
The next PR in this series will get to what I originally wanted to achieve and remove uses of the ugly "cast Dimension object to pointer" operators (see #2953) from libmoinfo.

User API & Changelog headlines

  • No API changes

Dev notes & details

  • Unused function MOInfoBase::correlate is removed
  • The only thing the MOInfoBase dtor does is call MOInfoBase::cleanup(), an empty function. Removed both.
  • Moved the contents of MOInfo::free_memory() into its only caller, the MOInfo dtor
  • Added a lot of new machine-readable docstrings, moved some existing ones
  • Some "obsolete code", commented out a long time ago is deleted from moinfo_mappings.cc
  • Moved the double** scf MO array and the get_scf_mos() getter from MOInfoBase to MOInfo and made the array private
  • Moved the contents of MOInfoBase::startup() into its only caller, the MOInfoBase ctor
  • Moved MOInfoBase::nmo to MOInfo and made it private
  • Made the molecular charge const and private in MOInfoBase
  • Made MOInfoBase::sopi private, added a protected getter fn to give access in derived classes
  • Made MOInfoBase::nso, MOInfoBase::nuclear_energy and MOInfoBase::irr_labs private
  • All functions in MOInfoBase that have previously returned copies of std::vector members now return const& to the vector in the object, avoiding a potential copy.
  • Since MOInfoBase::silent is only used in the ctor of MOInfo,, I removed it from MOInfoBase and MOInfoSCF
  • Inlined MOInfo::get_actv(size_t i) and MOInfo::get_docc(size_t i) as both were only called once. Since these overloads are now gone from MOInfo, it can now just use get_docc() and get_actv() from MOInfoBase instead of having to reimplement or explicitly inherit them.

Checklist

  • No new features
  • Tests run by the CI are passing
  • Errors in the full test suite appear to be unrelated to libmoinfo

Status

  • Ready for review
  • Ready for merge

@TiborGY TiborGY marked this pull request as ready for review April 28, 2024 01:47
TiborGY added 28 commits May 1, 2024 23:19
…le docstring, delete commented out "obsolete code"
…ectly accessed, return a const& for the array or an element
… variables are already named _ref to denote reference in a non-programming sense
@TiborGY TiborGY changed the title Moinfo pt2 Libmoinfo rework, part 2 May 5, 2024
Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

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

Thanks, this lgtm! Needs a review by someone closer to libmoinfo and mcscf, probably FAE.

@loriab loriab added this to the Psi4 1.10 milestone May 14, 2024
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

LGTM! @loriab, Francesco is going to be out of commission until June 8th, so I suggest we merge without him. Your call.

@loriab loriab added this pull request to the merge queue May 24, 2024
@loriab loriab added the cleanup For issues where the goal is to make Psi4 a little cleaner. label May 24, 2024
Merged via the queue into psi4:master with commit 8418efe May 24, 2024
5 checks passed
@loriab loriab mentioned this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup For issues where the goal is to make Psi4 a little cleaner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants