I don't understand this comment. Are the aug-dz and quasi-experimential numbers both provided here? Please elaborate so it's comprehensible to non-specialists.

This is a good point. I have included docstrings for both grac tests to clarify these values.

@pytest.mark.saptdft
def test_saptdft_auto_grac_iterative():
    """
    For SAPT(DFT), one must compute a GRAC shift for each monomer. Ideally,
    this GRAC shift should be close to the experimental Ionization Potential
    (IP) of the monomer. While the present test targets STO-3G, the IP for
    water is 0.1307. Note that using aug-cc-pVDZ, the computed GRAC shift is
    0.13063798506967816, which is close to the experimental value.
"""

Describe single and iterative... Also, it may be worth updating the SAPT(DFT) section of the documentation to describe this.

I have updated the description to be:

        /*- SAPT_DFT_GRAC_CONVERGENCE_TIER will specify how Psi4 should
          try to converge the cation for a GRAC shift before failing the
          calculation completely. "SINGLE" will try only once to converge 
          the cation for computing a GRAC shift. "ITERATIVE" will set adjust
          local Psi4 options ("LEVEL_SHIFT", "LEVEL_SHIFT_CUTOFF") to attempt
          to converge the neutral/cation calculations. "ITERATIVE" will
          try 3 times to converge the cation before failing the SAPT(DFT) 
          computation. -*/

Why do both monA and monomerA exist? I suspect that one of them needs to be renamed.

I split these into two different molecules because monomerA sets monomerB as ghosts; however, I want monA to not have ghost atoms to correctly compute the GRAC shift independent of the dimer. I can see how this is confusing, so perhaps I name it monA_grac?

That tells us what function uses it but not what it is. monomerA_all_bf, perhaps?

Are you sure you formatted this properly? Splitting parentheses across different lines so often is jarring.

I am using ruff's formatting defaults, but I will put all arguments on a single line if this is more preferable.

We have an (inactive?) PR to add Ruff formatting, #3234. The default Ruff formatting here is against the usual Psi4 style. I'm opposed to this, but a second opinion from another developer is good on style questions.

Including the Ruff formatting also made your PR harder to read, because now I need to separate out your code changes from your formatting changes.

Why have a function for this? This should be a module-level variable, no?

I agree that making it a module-level variable makes more sense.

How are you restoring these options to default later? Looking at the code, I can't see how you do it.

I was under the impression that optstash.restore() performs the resetting of options to the defaults; however, I did not all options that should be stored at the top of the function. Thank you for pointing this out.

These two tests are duplicates. Change them into one parameterized test.

I don't like having -99 as a magic number. I'm open to other opinions, but perhaps "DFT_GRAC_CONVERGENCE_TIER" should be renamed to "DFT_GRAC_COMPUTE" and take a "USER" value to specify that Psi should read these options.

Yeah, I also disliked a magical number; however, it seemed like a compromise for functionality of computing a grac shift for monomer A but not requiring it for monomer B and vice versa. I wouldn't want to force a user to compute a GRAC shift if they already have a shift for one of the monomers.

Perhaps this could be fixed by making SAPT_DFT_GRAC_SHIFT_A's default value of 0.0 enable "SAPT_DFT_GRAC_CONVERGENCE_TIER" (or the renamed form of "DFT_GRAC_COMPUTE") to "SINGLE" and enabling auto grac for A? Same logic would apply for monomer B.

That's a good idea, though I'd adjust the details slightly. If SAPT_DFT_GRAC_SHIFT_A is set (there's a special options method for that), we trust the user-provided value. Otherwise, we use the algorithm in SAPT_DFT_GRAC_CONVERGENCE_TEST. As you said, same logic applies for monomer B.

Oh, I was not aware of this special option... I just made a commit that checks to make sure the user has set the now "SAPT_DFT_GRAC_COMPUTE" variable to "SINGLE" or "ITERATIVE" from the default "NONE" in combination with SAPT_DFT_GRAC_SHIFT_A to 0.0. This might be similar to what you just recommended in functionality by ensuring that the user MUST request automatic GRAC shifts. The older error message of grac shifts being 0.0 and performing DFT has been updated to tell the user how to call the automatic GRAC shift computation.

It's best to query core.has_option_changed in a case like this... That way, we don't have to worry about how close is close enough, or what if the user set it to 0 manually.

Yeah, I agree that the "how close is close enough" is unsettling to use in practice; however, does this core.has_option_changed approach block users from running multiple SAPT(DFT) calculations in a row from one python script? For instance, compute_GRAC_shift() has to set "SAPT_DFT_GRAC_SHIFT_A" for later usage in the code. If I understand correctly, core.has_option_changed would now be triggered, effectively blocking the next SAPT(DFT) calculation from also automatically computing GRAC shifts. Is there a clean way to allow multiple auto GRAC SAPT(DFT) calls in a single python script to work correctly?

I largely ask because my high-throughput workflow would encounter this issue.

optstash.restore will restore the has_changed state from stash time.

Oh excellent, I have made this change to agree with the has_changed format. Thank you for your patience and quick responses on this issue!

That's what happens on a day when I'm waiting on cluster jobs to finish. By all means, test that your workflow still works after this change.

We have an (inactive?) PR to add Ruff formatting, #3234. The default Ruff formatting here is against the usual Psi4 style. I'm opposed to this, but a second opinion from another developer is good on style questions.

Including the Ruff formatting also made your PR harder to read, because now I need to separate out your code changes from your formatting changes.

That tells us what function uses it but not what it is. monomerA_all_bf, perhaps?

Having level_shift and its cutoff at the same value is unusual (grep of tests below). Should this be revised?

tests/scf-level-shift-cuhf/input.dat:set level_shift 0.5
tests/scf-level-shift-cuhf/input.dat:set level_shift_cutoff 1e-3
tests/scf-level-shift-rks/input.dat:set level_shift 5.0
tests/scf-level-shift-rohf/input.dat:set level_shift 0.7
tests/scf-level-shift-rohf/input.dat:set level_shift_cutoff 1e-4
tests/scf-level-shift-uhf/input.dat:set level_shift 0.5
tests/scf-level-shift-uhf/input.dat:set level_shift_cutoff 1e-5

This is a fair point. I will adjust these variables and run through some harder systems to converge to see how they perform. I ran about 4500 dimers with computing GRAC shifts with various level_shift and level_shift_cutoff values that will inform these updates. I am going to test using new values on a dataset before finalizing this response.

don't forget the glossary.rst entries

Would this be |sapt__sapt_dft_grac_shift_a| to |sapt__sapt dft grac shift a|?

yes, those two vars, but they'll look like the below. And put them somewhere in the SAPT area since the file doesn't alphabetize.

.. psivar:: SAPT DFT GRAC SHIFT A
   SAPT DFT GRAC SHIFT B

   The gradient-corrected ... [E_h] ...