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

SF-NOCI #126

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

SF-NOCI #126

wants to merge 10 commits into from

Conversation

seunghoonlee89
Copy link

This PR adds SF-NOCI, which was originally introduced by Mayhall and co-workers:

  • Phys.Chem.Chem.Phys. 16, 22694 (2014) (link)

And this PR is particularly based on a recent extension:

  • J. Chem. Theory Comput. ASAP (2025) (link)

for an efficient grouped bath ansatz.

We plan to add the following improvements based on this PR.

add examples and more unit tests (@jiseong0626)
add analytic energy gradient and X-ray spectrum calculation (@jiseong0626)
add DFT correction based on MC-PDFT (@BKim0828)

Copy link
Contributor

@matthew-hennefarth matthew-hennefarth left a comment

Choose a reason for hiding this comment

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

Any plans to include an example in the examples and unit tests for this method? Both would be good to have.

@@ -0,0 +1,702 @@
#!/usr/bin/env python
#
# Copyright 2024 The PySCF Developers. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change copyright to 2025.

@@ -0,0 +1,1270 @@
#!/usr/bin/env python
#
# Copyright 2024 The PySCF Developers. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change copyright to 2025.

@MatthewRHermes
Copy link
Collaborator

The failed CI unit tests are not your fault. Sorry, I keep overestimating the numerical precision of MC-PDFT on these runners. But please do fix the issues raised by the Lint.

Re unit tests and examples, I notice you already have an example calculation in a __main__ block in sfnoci.py. Perhaps you can copy that into a separate example file and turn it into a unit test.


def tearDownModule():
global mol, mo0, setocc, ro_occ
mol.stdout.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't close mol.stdout here. This object points to the sys.stdout

@@ -138,3 +138,12 @@ set_target_properties (clib_dsrg PROPERTIES
CLEAN_DIRECT_OUTPUT 1
LIBRARY_OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}
OUTPUT_NAME "dsrg")

Copy link
Contributor

Choose a reason for hiding this comment

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

Move these settings in a separated "CMakeLists.txt" file under the sfnoci directory. Then include this directory in this file

+ aa * ncas * ncas * ncas
+ ia * ncas *ncas
+ ab * ncas + ib]
* t1a[aa * ncas * na * na + ia * na * na + str1a * na + str0a]
Copy link
Contributor

Choose a reason for hiding this comment

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

aa*ncas*na*na and ia*na*na may cause integer overflow. Make a conversion such as (size_t)na for na

+ ia * ncas *ncas
+ ab * ncas + ib]
* t1a[aa * ncas * na * na + ia * na * na + str1a * na + str0a]
* t1b[ab * ncas * nb * nb + ib * nb * nb + str1b * nb + str0b]
Copy link
Contributor

Choose a reason for hiding this comment

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

integer overflow

+ i1 * ncas * ncas
+ a2 * ncas
+ i2]
* t2aa[a1 * ncas * ncas * ncas * na * na + i1* ncas * ncas * na * na + a2 * ncas* na * na + i2* na * na + str1a * na + str0a]
Copy link
Contributor

Choose a reason for hiding this comment

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

integer overflow

+ i1 * ncas * ncas
+ a2 * ncas
+ i2]
* t2bb[a1 * ncas * ncas * ncas * nb * nb + i1* ncas * ncas * nb * nb + a2 * ncas* nb * nb + i2* nb * nb + str1b * nb + str0b]
Copy link
Contributor

Choose a reason for hiding this comment

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

integer overflow

def python_list_to_c_array(python_list):
if python_list is None: return ctypes.c_void_p(None), ctypes.c_void_p(None), 0
else:
num_groups = len(python_list)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend to change this to np.ndarray. E.g.

python_list = [np.ndarray(x) for x in python_list]
flat_list = np.asarray(np.hstack(pythn_list), dtype=np.int32)
group_sizes = np.asarray([x.size for x in python_list], dtype=np.int32)

Then passing the .ctypes attributes of flat_list and group_sizes to the C functions

arrays.append(numpy.array(combination))
return arrays
result_arrays=find_arrays(n_as,n_ase)
array_list=[]
Copy link
Contributor

Choose a reason for hiding this comment

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

why not directly calling
concatenated_array = np.array(result_arrays, dtype=np.int32) ?

return concantenated_array

def fill_array_with_sum_N(length, N):
result = [0] * length
Copy link
Contributor

Choose a reason for hiding this comment

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

result = np.zeros(length, dtype=int)
...
    result[:N] = 1
else:
    result[:num_twos] = 2
    result[num_twos:] = 1

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.

5 participants