Skip to content

Pull request related to issue #1092 and #1093 - #1105

Closed
jvalegre wants to merge 0 commit into
cclib:masterfrom
jvalegre:master
Closed

Pull request related to issue #1092 and #1093#1105
jvalegre wants to merge 0 commit into
cclib:masterfrom
jvalegre:master

Conversation

@jvalegre

@jvalegre jvalegre commented Feb 8, 2022 •
edited
Loading

Copy link
Copy Markdown

Pull request related to issue #1092. The code broke due to a problem related to the mini print level of ORCA.

Changes:

  1. When parsing ORCA files using the mini print level, cclib raises a warning instead of breaking. The code prints a warning about this, but ideally the users should avoid the mini print level. For example, multiplicity just gets assigned to 1 since it's not included at this print level. The code doesn't break when using the mini level but the fix isn't ideal, since I set rmsDP_value and rmsDP_target to 0.0.
  2. Final single point E included. This is the final energy we use for single-point energy corrections in GoodVibes, but it wasn't included (I think).

I'm attaching the output files from ORCA and the json files generated with ccwrite, please let me know if you need more information!

TESTS.zip

Pull request related to issue #1093. There was a bug when retrieving the NBO (natural) charges for open-shell systems, since the code retrieved only the charges from the b-density rather than the total density. The code also includes new parameters in the json files generated with ccwrite from Gaussian calculations that GoodVibes and AQME use (already commented to @berquist):

  1. Parameters from NMR calcs
    image

  2. Metadata
    image

  3. Symmetry point group, rotational constants and T
    image

  4. Information about <S**2> before and after spin annihilation
    image

  5. Corrected natural (NBO) charges and spins
    image

  6. Wiberg bond order matrix (from NBO)
    image

Other fixes and additions:

  1. Thermochemistry (zpe, enthalpy, free energy) from Gaussian outputs was in hartree, now in eV
  2. Time dependent (TD) energies added from Gaussian outputs
  3. ONOIM energies added from Gaussian outputs
  4. G4 energies added from Gaussian outputs
  5. Final single-point energy added from ORCA outputs

@jvalegre jvalegre changed the title Pull request related to issue #1092 Pull request related to issue #1092 and #1093 Feb 9, 2022
@berquist berquist added this to the v1.7.2 milestone Feb 13, 2022
@shivupa
shivupa requested a review from berquist February 23, 2022 00:05
@berquist berquist self-assigned this Feb 23, 2022
@jvalegre

jvalegre commented Mar 19, 2022

Copy link
Copy Markdown
Author

hi @berquist , hope everything is going well!

I'd like to include similar data for ORCA and NWChem (I only added the GoodVibes parameters for Gaussian). Did you start working on this pull request or I can make a couple more changes to my local cclib code?

Thanks!

@langner langner modified the milestones: v1.7.2, v1.8 May 17, 2022
berquist
berquist requested changes Jun 4, 2022
View reviewed changes

@berquist berquist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The most important comment I can give is the one about breaking up this PR into smaller ones based on bug fixes and feature additions.

For the new attributes, #1093 (comment) and the documentation were not clear enough about

I see you put a bunch of stuff under the JSON path for metadata. This is good, but makes me realize that there is no guiding documentation about what to put there, and also that metadata is actually an attribute itself (the one linked above). Rather than let this review sit even longer, though we'll want to change a bunch of locations of things, can you split this PR apart first?

Comment thread cclib/parser/orcaparser.py Outdated
break
lines.append(line[line.find('> ')+2:])
if 'printlevel mini' in line or 'miniprint' in line:
print("WARNING: print level is set to mini, which might cause the loss of relevant information")

@berquist berquist Apr 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you use a logger warning instead?

@jvalegre jvalegre Jul 4, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread cclib/parser/gaussianparser.py Outdated
if "Zero-point correction" in line:
self.set_attribute('zpve', float(line.split()[2]))
zpve = utils.convertor(utils.float(line.split()[2]), "hartree", "eV")
self.set_attribute('zpve', zpve)

@berquist berquist Apr 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch on these.

@jvalegre jvalegre Jul 4, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks!

Comment thread cclib/parser/gaussianparser.py Outdated
charges.append(float(nline.split()[2]))
spins.append(float(nline.split()[-1]))
self.atomcharges["natural"] = charges
try:

@berquist berquist Apr 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think could lead to not saving the spins if they've been parsed but the attribute isn't there, so it should be something like

if spins:
    if not hasattr(self, "atomspins"):
        self.set_attribute("atomspins", {})
    self.atomspins["natural"] = spins

@berquist berquist Apr 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, could you refactor this and the line that gets the atomic charges so that there's only one nline.split() call and the tokens are reused?

@jvalegre jvalegre Jul 4, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I refactored it, however, this code comes from a previous version that doesn't separate Mulliken & NBO charges and spins (i.e. NBO charges and spins just override Mulliken values). Should I separate charges and spins into charges Mulliken, charges NBO, spins Mulliken and spins NBO?

@berquist berquist Jul 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should I separate charges and spins into charges Mulliken, charges NBO, spins Mulliken and spins NBO?

Yes please! This must have been an oversight and I can definitely see people would want clearly identified NBO properties. This means parsed output of any NBO calculation right now is incorrect.

@jvalegre jvalegre Jul 4, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

it's ready in PR #1141 - good thing is that this issue only caused problems in open-shell systems (i.e. wrong NBO charges and missing NBO spins). For closed-shell systems, older versions of cclib got the charges right and separated Mulliken from NBO

Comment thread cclib/parser/orcaparser.py Outdated

line = next(inputfile)
while len(line) > 20: # restricted calcs are terminated by ------
while len(line) > 20 and not 'Total SCF time' in line: # restricted calcs are terminated by ------

@berquist berquist Jun 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you break up this and the other fixes into separate PRs from the attribute and metadata additions? I will give an initial review for all of them in one pass right now, but it will be easier to have discussions with multiple things on multiple PRs.

@jvalegre jvalegre Jul 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yeah, let me distribute the fixes/additions into Gaussian, ORCA and NWChem (PRs #1141 , #1142 and #1143 )

@jvalegre jvalegre mentioned this pull request Jul 24, 2022
Open
@amandadumi amandadumi mentioned this pull request Jul 24, 2022
Open
@langner langner modified the milestones: v1.8, v1.8.1 May 30, 2023
@jvalegre jvalegre closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berquist berquist berquist requested changes

Assignees

@berquist berquist

Labels

Gaussian

Projects

None yet

Milestone

v1.8.1

Development

Successfully merging this pull request may close these issues.

3 participants

@jvalegre @berquist @langner