psi4 icon indicating copy to clipboard operation
psi4 copied to clipboard

Add customizable FREEZE_CORE policy option

Open tallakahath opened this issue 3 years ago • 8 comments

Description

This patch adds an option to FREEZE_CORE called "policy", which enables frozen core settings to be looked up from a list specified in the global variable FREEZE_CORE_POLICY. This is more flexible than NUM_FROZEN_DOCC for situations like SAPT where multiple molecules are run in the same command and may require different individual numbers of frozen cores, with settings more customizable than TRUE/FALSE/1/0/-1/-2

This patch addresses some (but not all) issues raised in #2631 by allowing for more flexible policies to be set appropriate to multi-part calculations.

Todos

  • [x] Add POLICY as option to FREEZE_CORE
  • [x] Add global variable FREEZE_CORE_POLICY to hold custom frozen-core policy

Checklist

  • [x] A functionality test for this flag has been added to tests/dfmp2-ecp/input.dat
  • [x] ctest -L quick runs successfully, which includes the above listed test
  • [ ] ctest still in-flight but given the scope of this patch I don't expect any issues

Status

  • [x] Ready for review
  • [ ] Ready for merge

tallakahath avatar Aug 08 '22 22:08 tallakahath

This is a little confusing to me.

It looks like basisset.cc line 240 accounts for Z = 0 but then the example array in the documentation at read_options.cc line 126 seems to have the Z = 0 in it. Or the example array could be mistyped (H-Be is just four atoms while five zeros are given). Then the example input file has the Sb atom (Z = 51) but core_policy[50] is used. I suspect users will be confused by this.

I'm not saying whether or not including Z=0 in the array is correct but documentation should be very clear.

jturney avatar Aug 09 '22 15:08 jturney

Or the example array could be mistyped (H-Be is just four atoms while five zeros are given).

Good catch! It was in fact a typo on my part, I've fixed it + added a little more clarification in the documentation for FREEZE_CORE_POLICY.

tallakahath avatar Aug 09 '22 15:08 tallakahath

Just one small addition to make and then it looks good to me.

jturney avatar Aug 09 '22 15:08 jturney

Updated!

Tbh, for large elements the use starts to get real clunky, but I don't understand the codebase well enough to instead provide a dict vs an array. Someone savvier than me should probably eventually update that (so that a user doesn't need to set 0's for a bunch of elements they Just Don't Care About), but I didn't see a mapping type available in the relevant context (just an int vector) so I went with what was there.

tallakahath avatar Aug 09 '22 15:08 tallakahath

Ah, yeah, for large elements it will get clunky. We do have MapType defined here. But looking through the code, it looks like we don't use it anywhere and therefore may or may not work. I think for now your solution is probably fine.

jturney avatar Aug 09 '22 15:08 jturney

I've authorized the test suite to run. This is our way of confirming that nothing is obviously broken. If everything passes (as I expect it should), there's nothing on your end yet to do. If it fails, give it a quick look and flag Lori or I if you need assistance in identifying the issue.

JonathonMisiewicz avatar Aug 11 '22 17:08 JonathonMisiewicz

@JonathonMisiewicz looks like most things worked except one of the linux builds failed for an issue that I don't think is related to my patch:

  Could NOT find Python (missing: Python_NumPy_INCLUDE_DIRS Interpreter
  NumPy) (found suitable version "3.8.10", minimum required is "3.8")

Not sure how to proceed here.

tallakahath avatar Aug 11 '22 18:08 tallakahath

I've restarted it, which has worked before. If it's the same problem as I've seen on Windows, it has to do with the latest NumPy. Haven't investigated why its sporadic on Linux yet though.

loriab avatar Aug 11 '22 18:08 loriab