rotcons isn't actually an attribute: it currently only exists as a calculation method. It needs to be added as a proper attribute. We also need to decide on what the standard units will be and potentially add any conversion factors to the converter (though Shiv already hinted at that).

Per #256 it seems like we were hoping users would calculate rotational constants from our method

Would this work with the expected use case you had in mind @amarkpayne

haha @berquist beat me to it

Well, I could see wanting to parse what's there rather than recomputing it. If you're doing this for a database (anything more than 100 logfiles, just to pick a number), the MOI tensor calculation and diagonalization will probably become significant in terms of time.

In our use case the user would be parsing hundreds of files, so speed would be preferable here. Let me also look at your method though

I agree then it would make sense to make this a parsed attribute.
The steps needed to do that would be to

1. Add an attribute and description to the ccdata object
2. Use self.set_attribute('rotcons', parsed_rotcons) to set the rotational constants that you parsed. A similar call for the masses.
3. A test for the dvb_sp.out would since this parses the line
   dvb_sp.out: Rotational constants (GHZ): 4.6266363 0.6849065 0.5965900
   So we could add a test in this file where the values of the parsed data is tested

Let us know if you get stuck at any point and we can help you

Edit:
And docs on this page
https://github.com/cclib/cclib/blob/master/doc/sphinx/data_notes.rst

So not to muddy the discussion but I actually was able to try out the Nuclear.rotational_constants method really quick (it wasn't too bad to code up, sorry I was unaware of its existence given that the MOPAC parser does things differently) and it seems to work just fine. In the unit tests we run, we only parse 4 logs, but as far as I can tell calculating these numbers did not significantly increase the time compared to everything else we are doing, and we get the same answers (at least within the precision we set in our unit tests).

TLDR it looks like either approach will work for us, so let me know what you prefer. Also let me know if molmass is fine as is as an attribute

Sorry to jump in, but it's also going to depend on the size of the molecule. The MOI and diagonalization is negligible for v. small molecules, but if you get to 100s of atoms, times 100s of log files, you'll want to parse it if available. For a database in the group, I'd definitely prefer to parse for formats where available.

TLDR it looks like either approach will work for us, so let me know what you prefer. Also let me know if molmass is fine as is as an attribute

It is up to you. In the short term you could proceed by using the Nuclear.rotational_constants method, and we could work on getting this in. If you didn't have the bandwidth to do this, we could take care of putting this in. Now that we know this would be of interest, it makes sense to work on it. Let us know what works for you.

I think we might not need molmass, since we could just sum atommasses (an existing attribute) whenever we need molmass?

Sorry to jump in, but it's also going to depend on the size of the molecule. The MOI and diagonalization is negligible for v. small molecules, but if you get to 100s of atoms, times 100s of log files, you'll want to parse it if available. For a database in the group, I'd definitely prefer to parse for formats where available.

Agreed!

@amarkpayne Poke. If you use utils.convertor to remove the hard-coded constants, then rebase (the test failures are not from your PR), I'd be happy to merge this.

Since Mark hasn't responded, I've decided to try finishing this off. It's also parsed for MOPAC, but as wavenumbers, so it needs a conversion. Right now that part of MOPAC isn't tested. I figured that should be in a follow-up PR.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (592a871) to head (9638d8d).
Report is 1205 commits behind head on master.

@@           Coverage Diff           @@
##           master    #1054   +/-   ##
=======================================
  Coverage   87.85%   87.86%           
=======================================
  Files          69       69           
  Lines       13503    13511    +8     
=======================================
+ Hits        11863    11871    +8     
  Misses       1640     1640           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

poke