🐛 `TrajectoryData`: Add `pbc`

[mbercx]

[!NOTE] There is a lot of code debt in the TrajectoryData, and the API is quite old and not optimal. Fixing all of that is beyond the scope of this PR, and probably we should redesign it in aiida-atomistic.

Fixes https://github.com/aiidateam/aiida-core/issues/6376 Fixes https://github.com/aiidateam/aiida-core/issues/7037

Pinging @danielhollas, @rikigigi and @dbidoggia for review, since they raised the original issues.

The current changes try to remain backwards-compatible for get_step_data, i.e. it now keeps on returning the same tuple of length 5. However, this does mean that the method does not return the pbc. For the get_step_structure method, I directly set the pbc from the property obtained from the attributes.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 79.60%. Comparing base (9b69202) to head (04cd6de). :warning: Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7079      +/-   ##
==========================================
- Coverage   79.61%   79.60%   -0.00%     
==========================================
  Files         566      566              
  Lines       43546    43536      -10     
==========================================
- Hits        34663    34654       -9     
+ Misses       8883     8882       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbidoggia]

Thanks @mbercx seems to be fine!

[mbercx]

Done! Check if you agree with the logic:

• If cells is None and
  • pbc is not specified (None) -> In this case we set pbc to [False, False, False]
  • pbc is equal to [False, False, False] -> All good
  • pbc is set to anything else -> raise. Can't have periodic boundary conditions without a cell, I suppose.
• If cells is not None and
  • pbc is not specified (None) -> In this case raise a deprecation warning that it should be specified and will be set to [True, True, True]
  • pbc is specified -> All good

I've also added tests for all cases. Not sure if a deprecation makes too much sense long term, as I noted above we will replace the TrajectoryData with a new version in aiida-atomistic.

[mbercx]

@danielhollas if you have a moment, could you have another look? We could still squeeze this into 2.7.2, since it's fixing a bug in my view. I want to avoid having this open too long and it falling through the cracks. 🙏

[danielhollas]

Yep, it's in my todo list for this week, thanks for the ping!

FWIW: I am not sure this should go to the patch release: it's a non-trivial change that fixes a bug that's been there forever so I think it's okay for it to be released in minor version, as it is basically adding new capability (and changes the API).

[mbercx]

Thanks @danielhollas! Round 3 done, let me know if there is anything else.

Also, do we need to update documentation somewhere? (I haven't checked)

TrajectoryData is rather succinctly documented I'm afraid, but I do not intend to fix that here.