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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hmacdope
Copy link
Contributor

@hmacdope hmacdope commented Nov 24, 2022

Description

Refactor electric_field() to return ElectricFieldInt rather than OneBodyAOInt

Fixes #2793

User API & Changelog headlines

  • ElectricFieldInt returned now by IntegralFactory.electric_field()
  • Equivalent changes in libmintshelper made to stop immediate static cast

Dev notes & details

  • ElectricFieldInt returned now by IntegralFactory.electric_field()
  • Equivalent changes in libmintshelper

Checklist

Status

  • Ready for review
  • Ready for merge

@maxscheurer
Copy link
Contributor

Isn't the purpose of the integral factory that one does not return the ptr to the specific integral class? Do we want to change the other integral types in the factory, too?

@hmacdope
Copy link
Contributor Author

Isn't the purpose of the integral factory that one does not return the ptr to the specific integral class? Do we want to change the other integral types in the factory, too?

I was just following a suggestion by @JonathonMisiewicz documented in #2793. Happy to close if required :)

@hmacdope hmacdope marked this pull request as ready for review November 27, 2022 01:02
@loriab loriab added this to the Psi4 1.8 milestone Nov 28, 2022
@JonathonMisiewicz
Copy link
Contributor

Isn't the purpose of the integral factory that one does not return the ptr to the specific integral class? Do we want to change the other integral types in the factory, too?

The point of the integral factory is that we don't need to pass the same arguments, over and over. Returning a pointer to the base class rather than the derived class gets us nothing. C++ will cast to the base class when necessary.

While I support changing the other integral types as well, I won't insist on it for this PR.

@hmacdope
Copy link
Contributor Author

hmacdope commented Dec 2, 2022

If so shall we push forward for this small change to avoid conflicts with #2775 that would occur if we changed everything to not return base class?

@JonathonMisiewicz
Copy link
Contributor

Fine by me.

@hmacdope
Copy link
Contributor Author

hmacdope commented Jan 6, 2023

@JonathonMisiewicz have we had any additional thoughts here? #2775 is probably a blocker

@JonathonMisiewicz
Copy link
Contributor

No. I agree that getting #2775 in first is best.

@JonathonMisiewicz
Copy link
Contributor

After fixing merge conflicts, then per discussion on #2775, there are other methods that need their return types adjusted.

@loriab loriab modified the milestones: Psi4 1.8, Psi4 1.9 May 4, 2023
@loriab loriab modified the milestones: Psi4 1.9, Psi4 1.10 Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make electric_field() return an ElectricFieldInt rather than a OneBodyAOInt
4 participants