change raw pointer return in `libmints` to `std::unique_ptr` #2775

hmacdope · 2022-11-14T11:44:07Z

Fixes #2493

Description

For memory safety, the integrals in libmints should be returned as unique_ptrs rather than raw pointers

User API & Changelog headlines

Dev notes & details

All integrals in libmints now return a unique_ptr rather than a raw pointer
Call sites refactored to match

Questions

Are my refactors to the call sites correct? Many are just an immediate release of the unique_pointer, with the assumption that the memory management / pointer deletion occurs elsewhere
Should iterators eg CartesianIter also return unique_ptrs?

Checklist

Tests added for any new features
All or relevant fraction of full tests run

Status

Ready for review
Ready for merge

hmacdope

Few questions @JonathonMisiewicz :)

psi4/src/psi4/libmints/mintshelper.cc

psi4/src/psi4/libmints/orbitalspace.cc

loriab · 2022-11-14T16:07:44Z

tests/test_erisieve.py is unhappy with the changes. You can run it locally via <objdir># pytest -v ../tests/pytests/test_eriseive.py after executing the results of <objdir># stage/bin/psi4 --psiapi.

hmacdope · 2022-11-14T21:31:02Z

tests/test_erisieve.py is unhappy with the changes. You can run it locally via <objdir># pytest -v ../tests/pytests/test_eriseive.py after executing the results of <objdir># stage/bin/psi4 --psiapi.

Thats strange, I couldn't reproduce locally ☹️

hmacdope · 2022-11-15T01:22:46Z

@JonathonMisiewicz @loriab I'm also a bit confused as to why none of the callsites for the eri integrals needed to be changes and everything compiled no sweat. I feel I am probably missing something?

loriab · 2022-11-15T01:26:43Z

hmm, do which psi4 python and python -c "import psi4;print(psi4.__file__, psi4.__version__)" to see if anything looks suspicious (or post them here if you're unsure).

hmacdope · 2022-11-15T01:29:09Z

python -c "import psi4;print(psi4.file, psi4.version)"

/Users/hugomacdermott/Desktop/dev/psi4_build/bin/psi4
/Users/hugomacdermott/miniconda3/envs/flow/bin/python
/Users/hugomacdermott/Desktop/dev/psi4_build/lib/psi4/__init__.py 1.7a1.dev109

But this should not affect the cmake compilation in my objdir?

hmacdope · 2022-11-15T01:31:51Z

@loriab to add to this the CI seems to build fine. Some of the tests no dice.

loriab · 2022-11-15T01:38:08Z

If /Users/hugomacdermott/Desktop/dev/psi4_build/ is your CMAKE_INSTALL_PREFIX, then this is fine, but you can make development a little easier by skipping the install step. In your objdir/ is a stage/ dir with a complete install that gets updated with every make. It's this installation that gets run with ctest. I'd seek that, then run the results of objdir/stage/bin/psi4 --psiapi and check that the which & python -c commands returns paths in objdir/stage/. This way there's a quick turnaround between edit/make/test.

I don't see anything necessarily wrong with what you posted. The above is just a setup that can reduce confusion if you forget the install step.

hmacdope · 2022-11-15T01:57:32Z

Ah yep thanks for the tips makes sense . It is my CMAKE_INSTALL_PREFIX.

I'm still confused though as to how it's even compiling

JonathonMisiewicz · 2022-11-15T13:29:47Z

@JonathonMisiewicz @loriab I'm also a bit confused as to why none of the callsites for the eri integrals needed to be changes and everything compiled no sweat. I feel I am probably missing something?

A quick grep tells me that the raw pointers returned by eri were converted to shared_ptr almost immediately. That's the reason why, I'm sure.

hmacdope · 2022-11-16T04:38:06Z

@JonathonMisiewicz @loriab I think I understand now, I thought you had to use an explicit std::move but that is only for an lvalue, you can do

std::shared_ptr<TwoBodyAOInt>(factory->erf_eri(omega_)

where erf_eri returns a std::unique_ptr because its a temporary rvalue

My bad sorry for the misunderstanding.

Regardless I will track down the failing test.

psi4/src/export_mints.cc

hmacdope · 2022-12-09T11:07:36Z

How do we want to move forward here @JonathonMisiewicz @loriab etc? :)

JonathonMisiewicz · 2022-12-22T11:26:23Z

Apologies for the delay; you decided to get this PR moving again when Lori and I were both extremely busy with PsiCon and 1.8.

The one outstanding issue here seems to be the Pybind holder conversion. I'll look that over later today.

JonathonMisiewicz · 2022-12-22T19:33:15Z

It looks like the segfault is a known Pybind issue. I think that the solution of changing the caster type from shared_ptr to unique_ptr is better coding practice. If nobody else objects, the Pybind conversion here seems as good as it's going to get.

Other thoughts? If nothing comes up soon, I'll give this another review.

JonathonMisiewicz · 2023-01-12T16:04:48Z

psi4/src/psi4/libmints/x2cint.cc

        OperatorSymmetry msymm(1, molecule_, integral_, soFactory_);
        std::vector<SharedMatrix> dipoles = msymm.create_matrices("Dipole");
-        OneBodySOInt *so_dipole = integral_->so_dipole();
+        std::unique_ptr<OneBodySOInt> so_dipole = integral_->so_dipole();


Can't we use auto here?

JonathonMisiewicz · 2023-01-12T16:08:06Z

psi4/src/psi4/fisapt/fisapt.cc

    auto Vfact = std::make_shared<IntegralFactory>(primary_);
    std::shared_ptr<PotentialInt> Vint;
-    Vint = std::shared_ptr<PotentialInt>(static_cast<PotentialInt*>(Vfact->ao_potential()));
+    Vint = std::shared_ptr<PotentialInt>(static_cast<PotentialInt*>(Vfact->ao_potential().release()));


Does Vint = Vfact->ao_potential() not suffice here (and many other places throughout this PR)?

Not without changing std::shared_ptr<PotentialInt> Vint; on L527, Im not libmints expert but does this have downstream effects or are we all good to cast to PotentialInt?

Lots of cases like this in this PR so probably good to decide what the strategy is.

To be more explicit: this line of code is both downcasting the underlying object and changing the type of the holding smart pointer to a shared pointer. I missed that first point in my original comment.

The underlying ao_potential method should be changed so that this downcasting isn't necessary, but that's beyond scope for this PR. This can stay as-is, begrudgingly, knowing that the removal of this ugliness is deferred to a follow-up PR. I'll do it if need be.

I see. This is in the same vein as #2793 and related PR #2795. Perhaps should make a more general issue. Happy to help out fixing.

JonathonMisiewicz

Now that the holidays are over (and I remembered about this PR, whoops), time to start getting this in.

JonathonMisiewicz

Good enough for now. The integral factory methods need to stop upcasting their return values in a different PR, but otherwise, LGTM.

JonathonMisiewicz · 2023-01-17T15:14:59Z

Is there a reason not to merge this now?

loriab

lgtm, thanks for the modernization.

JonathonMisiewicz · 2023-01-18T17:02:16Z

Is there a reason not to merge this now?

To be clear, this is directed at @hmacdope, since you don't have "Ready to merge" checked.

hmacdope · 2023-01-18T18:39:30Z

@JonathonMisiewicz oh sorry, yeah pits good to go

hmacdope added 8 commits November 13, 2022 18:42

start unique_ptr conversion

bdc92cf

ao kinetic and so kinetic

b60f2f4

ao_potential and so_potential

8fe1b52

ecp integrals

5c6da1d

relativistic and multipole

9a1161a

more integrals

07e2d77

fix sources

a27b48e

dinish ints

fcb0c7f

hmacdope commented Nov 14, 2022

View reviewed changes

hmacdope marked this pull request as ready for review November 14, 2022 11:49

apply changes from code review

a22719a

Merge remote-tracking branch 'upstream/master' into unique_ptr_libmints

caf9c01

change holder type for ERI to be a unique_ptr

c1b888a

hmacdope commented Nov 20, 2022

View reviewed changes

psi4/src/export_mints.cc Show resolved Hide resolved

hmacdope requested a review from JonathonMisiewicz November 20, 2022 13:37

hmacdope changed the title ~~WIP: change raw pointer return in libmints to std::unique_ptr~~ change raw pointer return in libmints to std::unique_ptr Nov 21, 2022

hmacdope mentioned this pull request Nov 23, 2022

Make electric_field() return an ElectricFieldInt rather than a OneBodyAOInt #2793

Open

loriab added this to the Psi4 1.8 milestone Nov 28, 2022

hmacdope mentioned this pull request Dec 2, 2022

Refactor libmints::IntegralFactory.electric_field() to return ElectricFieldInt #2795

Open

8 tasks

JonathonMisiewicz reviewed Jan 12, 2023

View reviewed changes

JonathonMisiewicz approved these changes Jan 13, 2023

View reviewed changes

jturney approved these changes Jan 14, 2023

View reviewed changes

loriab approved these changes Jan 17, 2023

View reviewed changes

JonathonMisiewicz merged commit 697fe5a into psi4:master Jan 18, 2023

loriab mentioned this pull request Jan 19, 2023

ints and master docs fixes #2855

Merged

7 tasks

change raw pointer return in libmints to std::unique_ptr #2775

change raw pointer return in libmints to std::unique_ptr #2775

Uh oh!

Conversation

hmacdope commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User API & Changelog headlines

Dev notes & details

Questions

Checklist

Status

Uh oh!

hmacdope left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loriab commented Nov 14, 2022

Uh oh!

hmacdope commented Nov 14, 2022

Uh oh!

hmacdope commented Nov 15, 2022

Uh oh!

loriab commented Nov 15, 2022

Uh oh!

hmacdope commented Nov 15, 2022

Uh oh!

hmacdope commented Nov 15, 2022

Uh oh!

loriab commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmacdope commented Nov 15, 2022

Uh oh!

JonathonMisiewicz commented Nov 15, 2022

Uh oh!

hmacdope commented Nov 16, 2022

Uh oh!

Uh oh!

hmacdope commented Dec 9, 2022

Uh oh!

JonathonMisiewicz commented Dec 22, 2022

Uh oh!

JonathonMisiewicz commented Dec 22, 2022

Uh oh!

JonathonMisiewicz Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

hmacdope Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

hmacdope Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

hmacdope Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz commented Jan 17, 2023

Uh oh!

loriab left a comment

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

change raw pointer return in `libmints` to `std::unique_ptr` #2775

change raw pointer return in `libmints` to `std::unique_ptr` #2775

hmacdope commented Nov 14, 2022 •

edited

Loading

hmacdope left a comment •

edited

Loading

loriab commented Nov 15, 2022 •

edited

Loading

JonathonMisiewicz commented Jan 18, 2023 •

edited

Loading