🐛 FIX: Add __str__ method to Orbital Class

[NinadBhat]

Fixes #4347

The absence of str was leading to recursion RecursionError #4347. So have added a str method.

[codecov]

Codecov Report

Merging #4829 (0a54de3) into develop (d35a9d7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4829      +/-   ##
===========================================
+ Coverage    79.54%   79.54%   +0.01%     
===========================================
  Files          519      519              
  Lines        37088    37092       +4     
===========================================
+ Hits         29497    29502       +5     
+ Misses        7591     7590       -1     

Flag	Coverage Δ
django	74.28% <100.00%> (+0.01%)	⬆️
sqlalchemy	73.19% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/tools/data/orbital/orbital.py	85.19% <100.00%> (+0.77%)	⬆️
aiida/transports/plugins/local.py	81.80% <0.00%> (+0.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d35a9d7...0a54de3. Read the comment docs.

[greschd]

I don't know our convention for what the result of __str__ should look like, maybe @ltalirz or @chrisjsewell can review this?

[sphuber]

There isn't a well defined convention yet, other than the Python standard that __str__ should be human readable for users and __repr__ is meant for debugging. For node instances (where there is little difference in the two cases I would say), we have been slowly converging on a format like:

NodeClass<IDENTIFIER>: CONTENT

so for this case, we could do something like

Orbital<1533>: [x, y, z](theta, phi, sigma)

For the identifier we typically take the PK. It is less transferable than the UUID, but the UUID is often very long and not always needed. I am not too familiar with the class itself, so the CONTENT I proposed is really just a rough idea adding the position vector and the angles and spin orientation in a separate vector. Feel free to adapt this as needed.

[sphuber]

Thanks @NinadBhat . I responded in a separate comment on the suggested format of the string returned by __str__. Let's wait to see if we can get confirmation from someone else. If they do, please change to that suggested format. Then simply add a test. As @greschd suggested, a simple test that just calls str(Orbital()) should be sufficient.

[sphuber]

Suggested change

	out_string = f' orbital @ {position_string}'
	return out_string
	return f' orbital @ {position_string}'

[ltalirz]

See also #3087

[NinadBhat]

@ltalirz I have made the changes.

[chrisjsewell]

this looks good to me, @ltalirz will merge later today unless you have any objections?

[sphuber]

@chrisjsewell in my review I suggested to change the output to Orbital<1533>: [x, y, z](theta, phi, sigma), or at the very least include the identifier of the node. Right now it is impossible to figure out which node is concerned

[chrisjsewell]

in my review I suggested to change the output

Ah fair, I missed that had not been addressed yet

[NinadBhat]

@chrisjsewell I am not sure how to access (theta, phi, sigma) and node in the Orbital class.

[greschd]

@sphuber note that this is the Orbital class, not OrbitalData - so these instances won't have a pk as far as I understand.

[sphuber]

@sphuber note that this is the Orbital class, not OrbitalData - so these instances won't have a pk as far as I understand.

Ah my bad! I completely missed this, I was indeed thinking of the OrbitalData. In that case maybe the current proposal is fine. @greschd as a user of the Orbital class, if you think the current __str__ format is fine, let's merge it.

[greschd]

@greschd as a user of the Orbital class

I'm not really using it directly, but it's definitely better than having #4347.