I think this is good to go, but I'm pushing it back to v1.5. I would also be nice perhaps to check the number of states/transitions here somehow corresponds to other attributes.

It occurred to me that we could do here what we did for atomcharges where there are also multiple ways to calculate the same thing. Namely, have one dictionary etdipoles with keys for the different forms/gauges. Does that sound reasonable?

I do, but what about the two extra columns for the electric dipole blocks compared to the magnetic dipole?

Well, the magnetic ones would have two fewer columns. We would also document the expected content of the columns (X-Z, S, osc).

Ok, I wasn't sure what you had planned for the internal structure.

Something like this:

print data.etdipoles
{'magnetic': <array 3xN>,
'electric': <array 5xN>}

I'm wondering whether it makes sense to include the state numbers the transitions are for in this attribute. What do you think?

I'm wondering whether it makes sense to include the state numbers the transitions are for in this attribute. What do you think?

I'm not sure it's necessary, since I don't think any programs allow for "skipping" this part of the calculation for any states since it comes at almost no cost compared to the excited-state calculation. On the off-chance that it is possible, since you can't have more transition dipoles than excited states, nan could be placed in the array.

As far as the structure of the attribute, here is what I am thinking. Since it is really transition properties (there may be a transition quadrupole moment, etc.) and not just dipoles, something more like data.transprop would be more general and allow for most programs to add the basics (transition electric dipole in length gauge, transition electric dipole in velocity gauge, transition magnetic dipole), while a more general program like Dalton could expand on it. The dictionary is also my preferred solution.

Because of my familiarity with Dalton, I prefer their language; it would look like:

data.transprop = {
    'diplen': np.array(shape=(nroots, 3)),
    'dipvel': np.array(shape=(nroots, 3)),
    'dipmag': np.array(shape=(nroots, 3)),
}

As far as the shapes, I think that the number of excited states should be along the first dimension, and then whatever makes sense in the remaining dimensions. I left the first two as having shape=(nroots, 3) because they can be calculated from the transition moment

as the square of the 2-norm:

and then with the excitation energy:

both of which would go nicely in the scripts/ directory.

I think I'm OK with you're proposed transprop, Eric, although I never liked that solution when we added it for atomcharges.

I left the first two as having shape=(nroots, 3) because they can be calculated from the transition moment

On second thought, we should parse everything available, because the precision of the dipole elements may be low, leading to a significant difference between what we would calculate and what is printed in an output file.

I mentioned in #398 (comment) that it may be beneficial to have very program-specific attributes in a dictionary (like you propose here), with more verbose names/keys. The attributes in this PR, however, are not program-specific, and it would be bad to mix dictionary key styles. Since these are more common, maybe it would be better to have them as specific top-level attributes, rather than in a dictionary?

On second thought, we should parse everything available, because the precision of the dipole elements may be low, leading to a significant difference between what we would calculate and what is printed in an output file.

I mentioned in #398 (comment) that it may be beneficial to have very program-specific attributes in a dictionary (like you propose here), with more verbose names/keys. The attributes in this PR, however, are not program-specific, and it would be bad to mix dictionary key styles. Since these are more common, maybe it would be better to have them as specific top-level attributes, rather than in a dictionary?

I'm opposed to creating multiple attributes for the same "thing". All these representation are, in principle, equivalent.

How about we just go forward with generalizing our attributes into classes? We could do it for this attribute now, and move atomcharges and whatever else would be backwards incompatible in 2.x. This will take some coding, but seem fairly straightforward. Actually, I see several options:

• turn attributes into classes (changes API a lot but provides lots of flexibility)
• make attributes into function calls (changes API, allows us to hide multiplicity)
• subclass int/float/ndarray/whatever and modify getitem (does not change API, hides multiplicity)

1. turn attributes into classes (changes API a lot but provides lots of flexibility)
2. make attributes into function calls (changes API, allows us to hide multiplicity)
3. subclass int/float/ndarray/whatever and modify getitem (does not change API, hides multiplicity)

I can try and make a mockup based on top of this PR. I don't like 3, because now is the only chance to change the API. I can't visualize what 2 would look like, is there anything it could do that 1 couldn't? I'm currently in favor of 1.

I think we should devise some temporary solution so as not to hold this feature up any longer, and just comment in the code that it will likely change like for other attributes. I'm going to move the discussion we started to #419.

Since it will be temporary, can we merge this as-is?

Since it will be temporary, can we merge this as-is?

Yeah, I'm OK with that. We just need to resolve the conflicts, and maybe add some comments. Will do before v1.5.2 goes out.

It looks like we can merge this in - I'll take one more look before finalizing v1.5.3.

Some conflicts need to be taken care of - pushing back.

Hmm.. this has been lingering for some time. The consensus here I think was to add these as temporary attributes. Originally metadata was supposed to hold tmp attribute, but I think it's evolved to something different now. @berquist thoughts? Merge as-is for now? We don't really have a convention about when an attribute is permanent... I'd like to have some idea about that first.

You're right. IIRC metadata was originally going to be more focused on input files, but now it is turning into a true metadata structure that should remain easily serializable. I don't remember any further discussion for metadata other than perhaps some of it should be split out into a separate input parser.

Here is a more concrete plan for transition properties. For the sake of merging, this PR looks overall fine to me. This should change for 2.0. Taking option 1 would allow for a very general container of transition properties that does not need to be dictionary based. For each state-to-state transition and for each operator, it would hold the transition energy, the operator moment components, the total oscillator strength, and the rotatory strength, depending on availability. Point group symmetry should also be considered.

Because of 2.0, we can skip the transprop dict entirely. Maybe the attribute is still called transprop. Keeping these individual attribute names for backwards compatibility might not be bad since they automatically come from most TDDFT calculations, and they can map easily to whatever the 2.0 representation is. The attribute is permanent once we have decided on its final representation.

What do we want to do here?

I think the action item here is to sync this with HEAD and merge it in as-is an updated PR. I can do this before the next release, looks simple enough. Last shout out to @mwykes in case he wants to take this up. I'd like to get the next version out before the end of March.

For the other issues, it looks like we've roughly converged on option 1 (extending all attributes into classes), which is a good general direction and perhaps will require some more design/discussion before we actually go through with it. This would be for 2.0 and we don't have a schedule for that yet.

Closing now that #803 has been merged.