Skip to content

Clean up #includes, macros, unnecessary array init, error exits, and other miscellaneous things in libmoinfo #3112

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.

Sign up for GitHub

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

Jump to bottom
Merged
loriab merged 37 commits into from
Apr 8, 2024
Merged

Clean up #includes, macros, unnecessary array init, error exits, and other miscellaneous things in libmoinfo #3112

loriab merged 37 commits into from
Apr 8, 2024

Conversation

@TiborGY
Copy link
Contributor

@TiborGY TiborGY commented Dec 17, 2023 •
edited
Loading

Description

This PR aims to make a lot of tiny improvements to libmoinfo.
While trying to modernize a bit of libmoinfo code to eliminate some (ab)use of the Dimension functions deprecated by PR #2953 I had to try to get at least some grip on what-does-what, but libmoinfo has accumulated some clutter which made that harder than necessary.
One thing led to another, and now this PR is here to hopefully make it easier for the next person.
Docstring addition and more are the subject of a future PR.

User API & Changelog headlines

  • No user-facing changes

Dev notes & details

  • Removed a some unused macro definitions, inlined the single use of #define size_det
  • Removed std::vector<size_t> ioff, its getter function, and initialization from MOInfoBase. It looks like this 5 M element array was getting non-trivially initialized every time an MOInfoBase object (or one derived from it) was constructed. RAM usage of these objects should now be 40 MB lower, and their construction should be faster by an unknown amount.
  • Removed some class declarations, included the appropriate .h instead where required instead of them (Note: some of this was reverted after reviews, see discussion on this PR.)
  • Touched-up what-includes-what, along the lines of removing what is not required, but adding what is
  • Removed declarations for tuning() and read_mo_spaces2(), as they are never defined
  • Removed some getters from MOInfo, either unused or already defined in the base class.
  • Removed unused fn MOInfoBase::get_irr_labs()
  • Renamed MOInfoBase::get_irr_labs(int i) to get_irr_lab, as it only gets one label. Rename propagated to all of its call sites.
  • Removed unused fn MOInfoBase::get_scf_mos(int i)
  • Moved the intvec and boolvec typedefs from global namespace, to namespace psi. Also modernized them to the using syntax.
  • Removed an unused extern ModelSpace* model_space declaration which was cluttering namespace psi
  • Replaced exit(PSI_RETURN_FAILURE) and exit(1) with throw PSIEXCEPTION Obviated by PR Do not call exit #3118

Checklist

  • No new features
  • Tests run by the CI were passing, but now they are failing for unclear reasons

Status

  • Ready for review
  • Ready for merge

@TiborGY TiborGY changed the title Clean up #includes, macros, some error exits, and other miscellaneous things in libmoinfo Clean up #includes, macros, unnecessary array init, error exits, and other miscellaneous things in libmoinfo Dec 20, 2023
@TiborGY TiborGY marked this pull request as ready for review December 21, 2023 16:48
loriab
loriab reviewed Mar 22, 2024
View reviewed changes
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.

Besides the forward decl issue, this looks reasonable to me. These are FAE modules, though, so will seek input. Thanks for the modernization!

@loriab loriab requested a review from fevangelista March 22, 2024 22:51
@fevangelista
Copy link
Contributor

fevangelista commented Mar 25, 2024

Thank you for making these improvements! This code was written before C++11 so it's really old style. I've left some comments here and there for things that need to be taken care.

fevangelista
fevangelista requested changes Mar 25, 2024
View reviewed changes
psi4/src/psi4/libmoinfo/moinfo.h

public:
typedef std::bitset<size_det> bitdet;
typedef std::bitset<2048> bitdet; // adjust based on "size det"?
Copy link
Contributor

@fevangelista fevangelista Mar 25, 2024

Choose a reason for hiding this comment

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

Just to double check: was size_det used only here? It might be a good idea to make this a static constant expression in the class SlaterDeterminant.

Copy link
Contributor Author

@TiborGY TiborGY Apr 5, 2024

Choose a reason for hiding this comment

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

Yes, this was the only hit for size_det in the entire repo. I have also searched for 2048 to check for things assuming this size, and found nothing. So having it "inline" like this seems like the most straightforward solution.

@TiborGY TiborGY force-pushed the libmoinfo_preproc branch 4 times, most recently from b0f64ac to 6bd09e7 Compare April 5, 2024 18:24
TiborGY added 16 commits April 8, 2024 19:22
@TiborGY
eliminate psifiles.h include in model_space_build.cc
3afedd0
@TiborGY
tidy moinfo.h #includes, remove unnecessary MOInfo declaration
7386d22
@TiborGY
remove unnecessary declaration: extern ModelSpace* model_space
7a4e6c6
@TiborGY
move the libpsi4util includes where they are needed
e44292e
@TiborGY
eliminate psifiles.h include in moinfo_model_space.cc
be1d4ac
@TiborGY
move wavefunction.h include from moinfo_scf.h to .cc
e47208a
@TiborGY
remove unnecessary libciomr and process.h includes from moinfo_scf.cc
aa52e1c
@TiborGY
remove unnecessary includes from moinfo.cc
60a10c6
@TiborGY
moinfo_base.h now needs to include <vector>
da19efb
@TiborGY
delete unused macro definitions from moinfo_base.h
693777e 
(cherry picked from commit f81c5f8)
@TiborGY
put the typedefs inside the psi namespace, and use "using" instead of…
b85db89 
… "typedef"

(cherry picked from commit 8779b4f)
@TiborGY
delete unused ioff array, its initialization, and getter
15c293a 
(cherry picked from commit 8eadfd2)
@TiborGY
add comment noting that psi::mcscf needs the "extern MOInfoSCF* moinf…
49424f6 
…o_scf" declaration
@TiborGY
psimrcc needs libpsi4util now
679dc80
@TiborGY
inline the size_det literal macro which only gets used once
d895319 
(cherry picked from commit 273a816)
@TiborGY
remove two declared, but never defined fns
e636540 
(cherry picked from commit 584c689)
TiborGY and others added 21 commits April 8, 2024 19:22
@TiborGY
remove unused get_scf_mos fns
82d1193 
(cherry picked from commit c11e558)
@TiborGY
changes from auto-formatter
7985333 
(cherry picked from commit 18c3471)
@TiborGY
remove unused fns get_extr and get_all
771af7d 
(cherry picked from commit 37245f3)
@TiborGY
remove unused fns get_mopi, get_focc, get_fvir
fe359ab
@TiborGY
use const & in MOInfoBase::read_mo_space
e7ed761
@TiborGY
try removing the Options and Wavefunction declarations from moinfo_ba…
d4bbdf6 
…se.h
@TiborGY
MOInfoBase::print_mo_space should take its agrs via const ref
8e018bd
@TiborGY
removed redefinition of MOInfo::get_sopi(), it is identical to the on…
f5a34c8 
…e defined in the base class

(cherry picked from commit c860f06)
@TiborGY
remove unused function MOInfoBase::get_ndocc()
6033fca 
(cherry picked from commit 081737c)
@TiborGY
remove unused get_irr_labs overload
07aa2c0 
(cherry picked from commit 4762398)
@TiborGY
rename get_irr_labs to get_irr_lab, as it only returns one label
7581825 
(cherry picked from commit a56b0aa)
@TiborGY
whitespace
a2a3734
@TiborGY
include wavefunction and liboptions headers to get back the class dec…
ac2f2ed 
…larations
@TiborGY
moinfo_scf.h does not need string
bcf671b
@TiborGY
try removing cstdio includes
8a09bfc
@TiborGY
try confining the ReferenceType enum inside the MOInfo class, so that…
f4cfd98 
… its keywords do not clutter the entire psi namespace
@TiborGY
Revert "try confining the ReferenceType enum inside the MOInfo class,…
6dae966 
… so that its keywords do not clutter the entire psi namespace"

This reverts commit 27b9178.
@TiborGY @loriab
Update psi4/src/psi4/libmoinfo/moinfo.h
9dcfe7a 
Lori's suggestion

Co-authored-by: Lori A. Burns <[email protected]>
(cherry picked from commit 180b8c4)
@TiborGY
revert model_space.h to fwd declare the MOInfo class instead of inclu…
2f6e412 
…ding moinfo.h
@TiborGY
revert moinfo_base.h to fwd declare the Options and Wavefunction clas…
71f321e 
…ses instead of including liboptions.h and wavefunction.h
@TiborGY
not including moinfo.h in model_space.h means model_space.cc and mode…
d55aae5 
…l_space_build.cc have to include moinfo.h again
@TiborGY TiborGY force-pushed the libmoinfo_preproc branch from bff3d39 to d55aae5 Compare April 8, 2024 17:22
@loriab loriab requested a review from fevangelista April 8, 2024 19:14
@loriab loriab added this to the Psi4 1.10 milestone Apr 8, 2024
@loriab loriab added this pull request to the merge queue Apr 8, 2024
Merged via the queue into psi4:master with commit 884f778 Apr 8, 2024
@TiborGY TiborGY mentioned this pull request Apr 25, 2024
Merged
20 tasks
@loriab loriab mentioned this pull request Jan 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loriab loriab loriab approved these changes

@fevangelista fevangelista fevangelista approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Psi4 1.10

Development

Successfully merging this pull request may close these issues.

3 participants

@TiborGY @fevangelista @loriab