Skip to content

Remove history method from RotatedSPOs - #5414

Merged
prckent merged 4 commits into
QMCPACK:developfrom
rcclay:remove_history_method
Apr 3, 2025
Merged

prckent merged 4 commits into
QMCPACK:developfrom
rcclay:remove_history_method

Conversation

@rcclay

@rcclay rcclay commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

I propose to remove the "history method" from the RotatedSPOs and only support the "global method". The history method of applying rotations in RotatedSPOs is not used in known production calculations. Beyond its initial use for having an independent orbital rotation implementation for code verification purposes, I don't really see a reason for keeping it around. Adds a lot of complexity to the RotatedSPOs class and obfuscates the logic.

What type(s) of changes does this code introduce?

  • Other (please describe): Remove unused feature

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

Intel Xeon workstation.

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes. This PR is up to date with current the current state of 'develop'
  • Yes. Code added or changed in the PR has been clang-formatted
  • No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • No. Documentation has been added (if appropriate)

@rcclay
rcclay requested review from camelto2 and ye-luo April 2, 2025 22:15
prckent
prckent previously requested changes Apr 2, 2025

@prckent prckent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camelto2

camelto2 commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

https://qmcpack.readthedocs.io/en/develop/intro_wavefunction.html?highlight=rotated#orbital-rotation mentions in the manual need cleaning. Also check Nexus.

I just took a look, and nexus does not have some preliminary support for rotated_sposets, but it doesn't allow the tags to distinguish between the global/history methods. So it would use the default, which is already global

camelto2
camelto2 previously approved these changes Apr 2, 2025

@camelto2 camelto2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other than the discussion in the manual

@rcclay

rcclay commented Apr 2, 2025

Copy link
Copy Markdown
Contributor Author

https://qmcpack.readthedocs.io/en/develop/intro_wavefunction.html?highlight=rotated#orbital-rotation mentions in the manual need cleaning. Also check Nexus.

That's a great catch. Removed the references to history method in the manual.

@rcclay
rcclay dismissed prckent’s stale review April 3, 2025 15:22

Required changes have been made.

@prckent prckent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.

I checked for but did not find any other mentions of method="history"

@prckent

prckent commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

Test this please

@prckent
prckent enabled auto-merge April 3, 2025 16:39
@prckent
prckent merged commit 1b3a0ea into QMCPACK:develop Apr 3, 2025
@rcclay
rcclay deleted the remove_history_method branch April 3, 2025 17:47
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.

4 participants