First of all, thanks for the contribution! Our convention has mostly been to parse the last (best?) results. So in this situation we would overwrite the attribute just as you've done, and add the regressions you need to cover ROCIS cases.

But, I would defer to Eric about the decision making here, I think he has more context and knowledge about these types of calculations. @berquist do you think parsing the final result is good here, or do we need to consider parsing multiple spectra for some reason?

Concerning the regression you broke - is that also ROCIS, causing it to fail?

Should I create a new set of regressions for ROCIS? It is very similar to TDDFT, but I need an open shell case to produce all the various different types of results to be printed.

What we do for unrestricted test cases is take the same divinylbenzene geometry from restricted cases and make the cation doublet. This may be an appropriate time to start incrementally making restricted open-shell tests. See more on this later.

What should be done about all of the different results being parsed? Just taking the last one currently breaks the tests.

See the next paragraph.

But, I would defer to Eric about the decision making here, I think he has more context and knowledge about these types of calculations. @berquist do you think parsing the final result is good here, or do we need to consider parsing multiple spectra for some reason?

I think that for now, we should test against results comparable to what we already have, which is ABSORPTION SPECTRUM VIA TRANSITION ELECTRIC DIPOLE MOMENTS, the origin-dependent length form, so the first result. This should maintain consistency for the current attributes across all programs, though I am not sure if some programs currently parse the velocity form rather than the length form (Gaussian?).

However, I would like to expand the types of properties we can parse, since multiple programs can print multiple forms. See a related old PR #227 that parses electric length, electric velocity, and magnetic transition dipole moments from Gaussian. This is good incentive to figure out how these less general attributes or properties should be stored.

This is (to me) just a ROTDDFT calculation with the TDA/CIS approximation and scaling factors for some integral terms. I think this test can be made more general (across parsers) if the molecule is switched from a boron atom to DVB, and the rest of the input is changed to something like

! roks b3lyp sto-3g nori noautostart

% rocis
    nroots  5
    maxcore 2500
    maxdim  50
    dori    false
    soc     true
    dohighermult true
    dodftcis   	true
    doquad      true
    dftcis_c = 0.21, 0.49, 0.29
    orbwin = 0, 0, 0, 2000
end

* xyz 1 2
 C                  0.27867948   -1.36683162    0.00000000
 C                  1.32303041   -0.44173575    0.00000000
 C                  1.04434506    0.92484978    0.00000000
 C                 -0.27867948    1.36683162    0.00000000
 C                 -1.32303041    0.44173575    0.00000000
 C                 -1.04434506   -0.92484978    0.00000000
 H                  2.36595443   -0.79037726    0.00000000
 H                  1.86746094    1.65407997    0.00000000
 H                 -2.36595443    0.79037726    0.00000000
 H                 -1.86746094   -1.65407997    0.00000000
 C                 -0.58659169    2.87589931    0.00000000
 C                  0.36350188    3.80076420    0.00000000
 H                 -1.65647768    3.12394312    0.00000000
 H                  0.14429560    4.87693235    0.00000000
 H                  1.43338788    3.55272039    0.00000000
 C                  0.58659169   -2.87589931    0.00000000
 C                 -0.36350188   -3.80076420    0.00000000
 H                  1.65647768   -3.12394312    0.00000000
 H                 -0.14429560   -4.87693235    0.00000000
 H                 -1.43338788   -3.55272039    0.00000000
*

At least DALTON, Q-Chem, I think NWChem, and presumably Gaussian can perform ROCIS/ROTDDFT calculations, and I think we should try and write a common test for all those parsers. Without running calculations it isn't obvious to me how sensitive the results will be to changing the dftcis_c parameters, which would affect if a single common test could be written with reasonable error bars. Maybe @jevandezande could shed some light on this.

Fix regression tests

It's only the old unit tests now in the regression suite, so whatever the code fix is will fix both the unit and regression tests.

Thanks for the input Eric, I totally missed the connection to the gauge thing.

The DFT/ROCIS I added is basically just ROCIS with DFT orbitals and some fudge factors. The scaling parameters included are the defaults; they have been optimized for B3LYP/def2-tzvp(-f) (Roemelt, M.; Neese, F. (2013) J. Phys. Chem. A, 117, 3069). If we wanted to simplify the test input, we could drop the DFT part and do standard ROCIS, which prints the exact same way. Since DVB is symmetric, the targeted core orbital must be localized, or the orbital window (OrbWin) needs to be expanded to include all symmetry equivalent orbitals.

I think saving all forms akin to PR #227 would be a great idea. This would make it easier to compare the effects of different approximations and also help protect the user from accidently grabbing the wrong absorption spectrum (that happened to me, which is what prompted the rewrite).

As a side note, such a module might be nice for thermochemistry, where there are a lot of small pieces that are spit out that someone may want to look at.

Ok, sorry for letting this sit for so long.

If we wanted to simplify the test input, we could drop the DFT part and do standard ROCIS, which prints the exact same way.

As long as the printing is exactly the same way, I'd like this, so it gives us a chance of reusing the test for other parsers. I discovered that Q-Chem can't do ROCIS, so this may be less relevant than I wanted.

Since DVB is symmetric, the targeted core orbital must be localized, or the orbital window (OrbWin) needs to be expanded to include all symmetry equivalent orbitals.

Can we get the same output from a non-XAS/valence-only calculation? I don't think other programs can do orbital window selection like ORCA, so they'd have to calculate all possible states, which might be expensive.

I think saving all forms akin to PR #227 would be a great idea. This would make it easier to compare the effects of different approximations and also help protect the user from accidently grabbing the wrong absorption spectrum (that happened to me, which is what prompted the rewrite).

A problem that I see here is that program-specific field names will be almost unavoidable as we move to less-general properties. Suggestions for what these should be named and how they're structured are welcome. If we're going to be program-specific, the overly verbose but safe solution is to use the section headers (X-RAY ABSORPTION SPECTRUM VIA TRANSITION ELECTRIC DIPOLE MOMENTS, etc.) as dictionary keys. That way, it's very clear what's been parsed without looking at the documentation.

If any of the above changes break possible added functionality and the test can't be made general, then I'd prefer the added functionality and an ORCA-specific test.

For now, if there is a section that's clearly identifiable as the preferable one, we should use that, and the general parsing strategy can be changed later when we make breaking changes.

As a side note, such a module might be nice for thermochemistry, where there are a lot of small pieces that are spit out that someone may want to look at.

If you could explain this in more detail (opening another issue for a question/discussion would be good), that would help our discussion about how "custom" or more complex attributes will be made user-facing in the future.

I added a transprop dictionary that has the title of each TDDFT, ROCIS, etc. calculation as the key and a tuple of (etenergies, etoscs) as the values. The etenergies and etoscs are then set to the last parsed spectrum. Thoughts?

As for testing, I added DVB with ROCIS and a test case subclassed from GenericTDTest. Then I overwrote most of the tests. I feel like keeping it with the TD test cases makes the most sense.

A problem that I see here is that program-specific field names will be almost unavoidable as we move to less-general properties. Suggestions for what these should be named and how they're structured are welcome. If we're going to be program-specific, the overly verbose but safe solution is to use the section headers (X-RAY ABSORPTION SPECTRUM VIA TRANSITION ELECTRIC DIPOLE MOMENTS, etc.) as dictionary keys. That way, it's very clear what's been parsed without looking at the documentation.

I think this could be dealt with easily if we expanded our concept of attribute. If used a custom Attribute class we could develop some convention (sub-attributes, lookups, dictionary-like behavior, whatever) that would cover less widespread variants or even program-specific results.

Now I'm unsure of what to do about this PR, since I don't want to make @jevandezande redo a lot of this work. Should we figure out the new structure before merging this, or take the "best result" and alter the structure to use the new API later?

Should the discussion of making a custom Attribute class be moved to a separate issue (it could potentially be used for other attributes)? Currently I only use ROCIS sparingly, so I am in no hurry to merge this code in.

Should the discussion of making a custom Attribute class be moved to a separate issue (it could potentially be used for other attributes)? Currently I only use ROCIS sparingly, so I am in no hurry to merge this code in.

I was worried about that, so this is good to hear. I've made a new issue: #419.

Now I'm unsure of what to do about this PR, since I don't want to make @jevandezande redo a lot of this work. Should we figure out the new structure before merging this, or take the "best result" and alter the structure to use the new API later?

Well, I'd also like this particular output to be supported, since it's needed. I'm OK with any of the temporary solutions we outlined above, new attribute or a dictionary - please implement what you guys think is most appropriate. But, could you please add a comment that it is likely to change in the future. I don't think it'll happen very soon, but there's a change we'll get to 2.x in 2018.

@jevandezande Any idea if you'll have the time to work on this some time soon, like in September?

I've added a comment about transprop being liable to change in cclib 2.0 (boring talk at the conference). Is this where you would like the comment? What else do I need to do to before we can get this pull request merged in?

I'm happy with things as they are now. @langner?

I'm OK with this as well. I'll leave the clicking to Eric.

Thanks for tolerating us, I'm glad we have a contributor that's making us think about more complicated stuff.