Fix bug in `TrajectoryData.get_step_data`

[lorisercole]

Fixes #4170

If a TrajectoryData does not contain an array cells the get_step_data method should define and return cell = None. There was a typo that caused an error in this case.

[sphuber]

Thanks @lorisercole . Could you rebase your branch on the latest develop such that the build is run correctly and maybe add a regression tests? It should go in tests.orm.data.test_trajectory.py which doesn't exist yet, but you can create it.

[codecov[bot]]

Codecov Report

Merging #4171 into develop will increase coverage by 0.33%. The diff coverage is 88.89%.

@@             Coverage Diff             @@
##           develop    #4171      +/-   ##
===========================================
+ Coverage    78.89%   79.21%   +0.33%     
===========================================
  Files          467      468       +1     
  Lines        34481    34481              
===========================================
+ Hits         27199    27311     +112     
+ Misses        7282     7170     -112     

Flag	Coverage Δ
#django	71.13% <88.89%> (+0.32%)	:arrow_up:
#sqlalchemy	71.97% <88.89%> (+0.28%)	:arrow_up:

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

Impacted Files	Coverage Δ
aiida/tools/data/array/trajectory.py	88.89% <ø> (ø)
aiida/orm/nodes/data/array/trajectory.py	46.75% <88.89%> (+2.61%)	:arrow_up:
aiida/engine/processes/workchains/workchain.py	85.42% <0.00%> (-4.73%)	:arrow_down:
.../migrations/versions/70c7d732f1b2_delete_dbpath.py	83.34% <0.00%> (-0.87%)	:arrow_down:
...ns/62fe0d36de90_add_node_uuid_unique_constraint.py	80.96% <0.00%> (-0.86%)	:arrow_down:
aiida/tools/dbimporters/plugins/pcod.py	43.91% <0.00%> (-0.74%)	:arrow_down:
aiida/backends/sqlalchemy/migrations/env.py	80.65% <0.00%> (-0.60%)	:arrow_down:
aiida/tools/dbimporters/plugins/icsd.py	21.14% <0.00%> (-0.37%)	:arrow_down:
aiida/engine/processes/calcjobs/calcjob.py	82.02% <0.00%> (-0.31%)	:arrow_down:
aiida/orm/nodes/data/array/bands.py	75.96% <0.00%> (-0.25%)	:arrow_down:
... and 120 more

---

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 c817021...09faa60. Read the comment docs.

[lorisercole]

Sure @sphuber. I've done the rebase. What kind of tests should I add? I see there is already something in tests.orm.data.test_data.py. Maybe we should test these methods to extract data or a structure from a particular step of the trajectory. Anything else?

[sphuber]

Those tests are only to test the export functionality. What would be best is to create the file tests.orm.data.test_trajectory and add some basic tests. At the very least create a test that would trigger the exception that you uncovered. So something like:

def test_trajectory_get_step_data_empty():
    trajectory = TrajectoryData()  # Create without cells, times, etc.
    result = trajectory.get_step_data()  # Check that this doesn't raises
    assert result[2] == None  # Or whatever the index of cells is in the index. Just check the return value is what you expect

[lorisercole]

I added some tests for TrajectoryData. I test the get_step_data, get_step_structure, and get_index_from_stepid methods. Are these enough or are there other methods that should be tested?

To compare arrays (positions/velocities/cells) I converted them to lists. Don't know if there is a better way to do this. Numpy offers something simpler: e.g. numpy.testing.assert_array_equal. Also, is there a better way to compare two StructureData nodes?

[lorisercole]

I added the optional parameter custom_cell to TrajectoryData.get_step_structure, that allows one to override the cell of the structure that is be extracted from a TrajectoryData. If omitted, the cell will be read from the trajectory, if present. If a cell is not found, the StructureData will use the default cell, and an user warning will be emitted. I added a test for this functionality. Ready for review.

[sphuber]

Since this PR has been open for a long time, I have taken the liberty to rebase it on the current latest main and just keep the fix for #4170 in a new PR #5734 . I have only kept the immediate bug fix and not included the added feature of the custom_cell for the get_step_structure method. The reason is that there were open questions on its use and if a user doesn't want the default cell, it can be easily changed after the return:

structure = trajectory.get_step_structure(1)
structure.cell = some_other_cell

The get_step_structure returns an unstored node, so this is possible without any issues. @lorisercole if you or anyone else still thinks that the argument is a better/required solution, please open a separate PR.