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

Odometer tester #55

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Odometer tester #55

merged 1 commit into from
Jul 9, 2024

Conversation

alnaba1
Copy link
Collaborator

@alnaba1 alnaba1 commented Apr 25, 2024

@msricher Made a test for odometer one spin for H2 system in the minimal basis, I would expect that when we pass cost of the orbitals as energies: [0.74587179 1.10115033], if q_max is bigger than 0.75 it would select only the first determinant. If it is bigger than 1.2, it would select all determinants.

Currently, selecting a value lower than 0.74, doesn't select any determinant as expected. However, if it is bigger than 1.2 currently then we get an index error (or a segmentation fault error when we tried with the H4 system). Can you help to figure out what's going on?

@msricher
Copy link
Collaborator

It's because you are using a fullci_wfn, for which the arguments to add_occs are a (2, $N_\text{up}$) ndarray of c_ints. I changed your code in pyci/utility.py to (starting from line 176)

    # Select determinants
    while True:
        if new[-1] < wfn.nbasis and (np.sum(cost[new]) + t * cost[new[-1]]) < qmax:
            # Accept determinant and go back to last particle
            wfn.add_occs(np.vstack((new, new)))
            j = wfn.nocc_up - 1
        else:
            # Reject determinant and cycle j
            new[:] = old
            j -= 1
...

and it works:

[1+]-[*36-cost-tester]-[michelle@t14s-g4]-[~/Git/pyci]-% pytest -sv pyci/test/test_odometer.py
================================================= test session starts =================================================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.3.0 -- /home/michelle/Git/venv/bin/python
cachedir: .pytest_cache
rootdir: /home/michelle/Git/pyci
plugins: anyio-4.2.0
collecting ... converged SCF energy = -1.06610864931794
converged SCF energy = -1.06610864931794
converged SCF energy = -2.09854593699772
collected 3 items                                                                                                     

pyci/test/test_odometer.py::test_odometer_one_spin[wfn0-cost0-0-0.7] PASSED
pyci/test/test_odometer.py::test_odometer_one_spin[wfn1-cost1-0-1.0] PASSED
pyci/test/test_odometer.py::test_odometer_one_spin[wfn2-cost2-0-1.2] PASSED

================================================== 3 passed in 0.72s ==================================================

You should check if the wfn instance is a one-spin (doci or genci) or two-spin (fullci) and if it's two-spin, do as I showed here.

@msricher
Copy link
Collaborator

msricher commented Apr 26, 2024

Actually an easier solution is just, start with a doci_wfn, run your odometer algorithm to fill it, and then upcast it to a fullci_wfn:

wfn = pyci.doci_wfn(nbasis, n_up, n_dn)
# fill `wfn`
# ...
wfn = pyci.fullci_wfn(wfn)

You can upcast {doci,fullci}_wfn to genci_wfn, and doci_wfn to fullci_wfn. This involves copying the wfn, though, so if it's a gigantic wfn (billions of dets) this isn't a good idea.

@alnaba1
Copy link
Collaborator Author

alnaba1 commented May 26, 2024

@msricher Is there a simple way of checking if the wfn instance is one-spin or two-spin? I've updated the Odometer algorithm to check the type of the wfn object using isinstance() but it doesn't seem elegant.

My concern with doing it this way is that we have to list out every possible type of wfn object and check if the instance of wfn is of the one-spin or two-spin type, and I'm not sure if there would be more types of wfn objects that would be implemented later and not be included in the types I have currently listed.

    if isinstance(wfn, pyci.fullci_wfn):
        two_spin = True
    elif isinstance(wfn, (pyci.doci_wfn, pyci.genci_wfn)):
        two_spin = False
    else:
        raise TypeError
    while True:
        if new[-1] < wfn.nbasis and (np.sum(cost[new]) + t * cost[new[-1]]) < qmax:
            # Accept determinant and go back to last particle
            if two_spin:
                wfn.add_occs(np.vstack((new, new)))
            else:
                wfn.add_occs(new)

@msricher
Copy link
Collaborator

This is fine, there's only three classes, so you can check this way. There won't be more added.

@msricher
Copy link
Collaborator

msricher commented May 28, 2024

You can also just use the abstract superclasses pyci.one_spin_wfn and pyci.two_spin_wfn. isinstance(dociwfn, pyci.one_spin_wfn) should be true etc.

Copy link
Collaborator

@msricher msricher left a comment

Choose a reason for hiding this comment

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

This looks ok to me. If the tests run, you can merge it, although I'm not as familiar with this work being done, so I'd wait for Rik to approve it.

Can you rename the h{2,4}.fcidump files to specify the basis set? E.g. h2_sto3g.fcidump.

Copy link
Collaborator

@RichRick1 RichRick1 left a comment

Choose a reason for hiding this comment

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

Good job! There are few things that needs to be fixed:

  1. I don't think there is a need to check a way function type in the odometer_one_spin algorithm. It is used only for the one_spin wavefunction and this can be checked directly in the test function when odometer_one_spin or odometer_two_spin is called.
  2. Test if the expected orbitals are selected when custom cost is provided
  3. Fix the test that throws the error on GH action. There is something wrong with a missing file

pyci/test/test_odometer.py Outdated Show resolved Hide resolved
pyci/utility.py Outdated Show resolved Hide resolved
pyci/utility.py Outdated Show resolved Hide resolved
@alnaba1
Copy link
Collaborator Author

alnaba1 commented Jun 7, 2024

All finished, ready to merge.

@msricher
Copy link
Collaborator

msricher commented Jul 8, 2024

OK, yes this is ready to be merged. Can you just rebase it on master? Then I'll merge it.

@alnaba1 alnaba1 force-pushed the 36-cost-tester branch 2 times, most recently from 1a6c705 to 42d2af1 Compare July 8, 2024 23:45
@alnaba1
Copy link
Collaborator Author

alnaba1 commented Jul 9, 2024

OK, yes this is ready to be merged. Can you just rebase it on master? Then I'll merge it.

@msricher All done.

@alnaba1 alnaba1 merged commit 73bfa97 into master Jul 9, 2024
1 check passed
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.

None yet

3 participants