Skip to content

Remove unused and pointer-argumented overloads of Matrix::diagonalize #2693

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
Closed
wants to merge 8 commits into from
Closed

Remove unused and pointer-argumented overloads of Matrix::diagonalize #2693

wants to merge 8 commits into from

Conversation

TiborGY
Copy link
Contributor

@TiborGY TiborGY commented Aug 18, 2022 •
edited
Loading

Description

Some overloads of Matrix::diagonalize in libmints are never called anywhere, they are deleted.
On the suggestion of @JonathonMisiewicz I have only kept the overloads that do not take pointer arguments and modified the call sites where required.
This is another shard of the #2642 mega-PR that can be merged independently.

Todos

  • Unused Matrix::diagonalize overloads are gone
  • Matrix::diagonalize no longer takes pointer arguments

Status

  • Ready for review
  • Ready for merge

@TiborGY TiborGY mentioned this pull request Aug 18, 2022
Merged
13 tasks
@TiborGY TiborGY changed the title Remove unused overloads and pointer arguments of Matrix::diagonalize Remove unused and pointer-argumented overloads of Matrix::diagonalize Aug 18, 2022
@TiborGY TiborGY marked this pull request as ready for review August 18, 2022 17:01
loriab
loriab reviewed Aug 19, 2022
View reviewed changes
@TiborGY TiborGY mentioned this pull request Aug 19, 2022
Draft
39 tasks
loriab
loriab reviewed Sep 23, 2022
View reviewed changes
loriab
loriab approved these changes Sep 23, 2022
View reviewed changes
@loriab loriab added this to the Psi4 1.7 milestone Sep 23, 2022
@loriab loriab added libmints For things that are wrong with libmints (but not wavefunction). cleanup For issues where the goal is to make Psi4 a little cleaner. labels Sep 23, 2022
@JonathonMisiewicz
Copy link
Contributor

I'm having second thoughts about this PR now. Should we deprecate the old constructors first? This PR will require changes in Forte as well.

@TiborGY
Copy link
Contributor Author

TiborGY commented Sep 23, 2022

I'm having second thoughts about this PR now. Should we deprecate the old constructors first? This PR will require changes in Forte as well.

Hmm, indeed while the functions are not PSI_API, the entire Matrix class is. Ouch. This is something I have completely missed, deprecation before removal is something I agree with when it comes to APIs.

The question is, should "deprecate before removal" principle be applied to all member functions/ctors of a 'PSI_API` class? I would assume yes.

@loriab
Copy link
Member

loriab commented Sep 23, 2022

I'm having second thoughts about this PR now. Should we deprecate the old constructors first? This PR will require changes in Forte as well.

options as I see them. I do think we should try to get the changes into one psi4 release cycle.

  • (n/c) leave convenience fns as-is
  • (deprecate) leave convenience fns operable but add deprecation message
  • (upgradehelper) leave convenience fn header in place but have it print a message with necessary changes
  • (remove) remove convenience fn header and body

@JonathonMisiewicz
Copy link
Contributor

I vote deprecate, just in case we change our mind on this decision.

@loriab
Copy link
Member

loriab commented Sep 23, 2022

I vote deprecate, just in case we change our mind on this decision.

fine by me

@TiborGY
Copy link
Contributor Author

TiborGY commented Sep 23, 2022

Agreed on the deprecation.

@TiborGY TiborGY marked this pull request as draft September 23, 2022 18:25
@TiborGY
Copy link
Contributor Author

TiborGY commented Sep 24, 2022

I do think we should try to get the changes into one psi4 release cycle.

Regarding the deprecation message/schedule: do you mean that they should be deprecated now and then removed before 1.7 RC1?

@loriab
Copy link
Member

loriab commented Sep 27, 2022

I do think we should try to get the changes into one psi4 release cycle.

Regarding the deprecation message/schedule: do you mean that they should be deprecated now and then removed before 1.7 RC1?

I think deprecation warnings (with code still functional) need to be in at least one release. So the code can be broken as soon as ~Dec (after 1.7.0 release).

But what I was meaning to say with "get the changes into one psi4 release cycle" was let's aim to get all the immediate-breaks and/or notifications in before 1.7.0 so that downstream users have the info to do a single overhaul after 1.7.0. Keep pinging me if this doesn't make sense :-)

@TiborGY TiborGY mentioned this pull request Oct 7, 2022
Merged
11 tasks
@TiborGY
Copy link
Contributor Author

TiborGY commented Oct 18, 2022

I propose that this PR could be re-tagged as Psi4 1.8 and kept open after #2738 is merged, as a reminder to delete the functions.

@loriab loriab modified the milestones: Psi4 1.7, Psi4 1.8 Nov 29, 2022
@TiborGY TiborGY mentioned this pull request Dec 17, 2022
Merged
6 tasks
@loriab loriab closed this in #2837 Jan 19, 2023
@TiborGY TiborGY deleted the patch-11 branch January 22, 2023 16:42
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

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz approved these changes

Assignees
No one assigned
Labels
cleanup For issues where the goal is to make Psi4 a little cleaner. libmints For things that are wrong with libmints (but not wavefunction).
Projects
None yet
Milestone
Psi4 1.8
Development

Successfully merging this pull request may close these issues.
3 participants
@TiborGY @JonathonMisiewicz @loriab