Skip to content

Modernize BasisSet::print_detail_cfour #2937

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 16 commits into from
Nov 14, 2023
Merged

Modernize BasisSet::print_detail_cfour #2937

loriab merged 16 commits into from
Nov 14, 2023

Conversation

@TiborGY
Copy link
Contributor

@TiborGY TiborGY commented Apr 30, 2023 •
edited
Loading

Description

Much like PR #2804, this PR is mostly about eliminating sprintf-related compiler warnings and modernizing C-style string handling to C++, this time in libmints' BasisSet::print_detail_cfour.

Dev notes & details

  • Modernized BasisSet::print_detail_cfour(), highlights:
  • No longer uses sprintf and a fixed-size char array
  • Variables are made const wherever reasonable
  • Use of std::find on doubles is replaced by new functions (none_of_equal and fpeq) that implement a fuzzy compare with a default tolerance of 1E-14. Probably no behaviour change in practice (unless someone has basis fns which differ in exponent by >0 but <1E-14), but it is usually not recommended to use == on FP types, which is what std::find does.
  • The slater.chemie.uni-mainz.de URL has succumbed to link rot. Replaced with one that points to the latest version that is being preserved by the Internet Archive, both in the C++ and Python implementations.
  • Ran clang-format on basisset.cc and basisset.h

Questions

  • The new functions none_of_equal and fpeq are just living in basisset.cc and basisset.h right now. Would some other file be a better place to put them?

Checklist

  • No new features
  • Tests run by the CI are passing
  • I do not have CFOUR, so I cannot really test if this breaks something. It should not, but the std::find change is behaviour-altering, albeit only in a corner case.

Status

  • Ready for review
  • Ready for merge

@TiborGY TiborGY force-pushed the basisset_renovation branch from 1b94d93 to ccb6f5c Compare April 30, 2023 15:22
@loriab
Copy link
Member

loriab commented Apr 30, 2023

Formatting is great, but just in case part of your goal is to compare basissets at the .gbs file level, I wanted to let you know about https://github.com/psi4/psi4/blob/master/psi4/share/psi4/basis/primitives/diff_gbs.py . Thhe basis set exchange has one, too, -- I think https://github.com/MolSSI-BSE/basis_set_exchange/blob/master/basis_set_exchange/curate/diff.py

I'll look into the adcc errors. I think Max updated the conda packages recently.

@TiborGY
Copy link
Contributor Author

TiborGY commented Apr 30, 2023

This branch was waiting for #2804 to be merged, the original goal was just to properly fix for some compiler warnings, but I will need to re-read these changes myself, it has been waiting for months.

@TiborGY TiborGY force-pushed the basisset_renovation branch from 61bfdb7 to 4ee65c3 Compare May 6, 2023 14:42
@TiborGY TiborGY marked this pull request as ready for review May 6, 2023 16:44
@TiborGY TiborGY changed the title Basisset renovation Modernize BasisSet::print_detail_cfour May 6, 2023
@TiborGY TiborGY force-pushed the basisset_renovation branch from a5e71dd to 2606fc0 Compare May 24, 2023 11:58
loriab
loriab approved these changes Nov 14, 2023
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.

Nice modernization, thank you!

I've run the tests/cfour test suite and a partial cfour ccsd(t) many-body job with lots of ghost atoms, and all pass, so lgtm.

@loriab
Copy link
Member

loriab commented Nov 14, 2023

Conflicts are trivial, so please lmk if you want to handle the rebase or website UI resolve, or if I should, @TiborGY .

@loriab loriab added this to the Psi4 1.9 milestone Nov 14, 2023
@TiborGY
Copy link
Contributor Author

TiborGY commented Nov 14, 2023 •
edited
Loading

Conflicts are trivial, so please lmk if you want to handle the rebase or website UI resolve, or if I should, @TiborGY .

Thanks for the testing, conflicts resolved via the web-UI.
PS. Github decided to do a "merge-master-into-this" to resolve the conflict, LMK if that is a problem.

@loriab loriab added this pull request to the merge queue Nov 14, 2023
Merged via the queue into psi4:master with commit 0d54746 Nov 14, 2023
@TiborGY TiborGY deleted the basisset_renovation branch November 19, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jturney jturney jturney approved these changes

@loriab loriab loriab approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Psi4 1.9

Development

Successfully merging this pull request may close these issues.

3 participants

@TiborGY @loriab @jturney