-
Notifications
You must be signed in to change notification settings - Fork 440
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
change raw pointer return in libmints
to std::unique_ptr
#2775
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions @JonathonMisiewicz :)
tests/test_erisieve.py is unhappy with the changes. You can run it locally via |
Thats strange, I couldn't reproduce locally |
@JonathonMisiewicz @loriab I'm also a bit confused as to why none of the callsites for the |
hmm, do |
But this should not affect the cmake compilation in my objdir? |
@loriab to add to this the CI seems to build fine. Some of the tests no dice. |
If 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. |
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 |
A quick grep tells me that the raw pointers returned by |
@JonathonMisiewicz @loriab I think I understand now, I thought you had to use an explicit std::shared_ptr<TwoBodyAOInt>(factory->erf_eri(omega_) where My bad sorry for the misunderstanding. Regardless I will track down the failing test. |
libmints
to std::unique_ptr
libmints
to std::unique_ptr
How do we want to move forward here @JonathonMisiewicz @loriab etc? :) |
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. |
It looks like the segfault is a known Pybind issue. I think that the solution of changing the caster type from Other thoughts? If nothing comes up soon, I'll give this another review. |
@@ -144,7 +144,7 @@ void X2CInt::compute_integrals() { | |||
if ((lambda_[0] != 0.0) or (lambda_[1] != 0) or (lambda_[2] != 0)) { | |||
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use auto
here?
@@ -525,7 +525,7 @@ void FISAPT::nuclear() { | |||
|
|||
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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Vint = Vfact->ao_potential()
not suffice here (and many other places throughout this PR)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of cases like this in this PR so probably good to decide what the strategy is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the holidays are over (and I remembered about this PR, whoops), time to start getting this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough for now. The integral factory methods need to stop upcasting their return values in a different PR, but otherwise, LGTM.
Is there a reason not to merge this now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for the modernization.
To be clear, this is directed at @hmacdope, since you don't have "Ready to merge" checked. |
@JonathonMisiewicz oh sorry, yeah pits good to go |
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
Questions
CartesianIter
also return unique_ptrs?Checklist
Status