Clean up Trajectory

[mjwen]

Summary

This PR cleans up the Trajectory class:

• add type annotation
• make doc string consistent with what the code is actually doing
• remove unused code pieces and slightly reorganize some parts

This breaks backward compatibility in one place. Previous frame_properties accepts a dict as input and properties of different frames are gathered in a sequence. Now, it has a similar signature as site_properties, that is, it accepts a list of dict, one for each frame. By doing this, the frame_properties can accommodate more use cases, e.g. accepting nested dict as frame properties, also, this makes extending and slicing easier.

[coveralls]

Coverage decreased (-20.5%) to 63.575% when pulling 9d8a0cfc1b4e8fcc5e037a9aadf30c8a474a572a on mjwen:master into 2b75957884934470962b56c644588eba1454dbaa on materialsproject:master.

[sivonxay]

The changes look good so far. They make things more clear/transparent about what's happening.

While this isn't being used much in our group at the moment, It's unclear to me how many people are using it by external users of pymatgen. What did you have in mind for changes to to_positions() and to_displacements()? I'd be a bit cautious to make changes that change the fundamental operation of the class.

P.S. On line 167, regarding your comment about figuring out what that code is doing: It's correcting the displacements to account for periodic boundary conditions (in a wrapped simulation). For example - If in one frame an atom has fractional coordinates of [0, 0, 0.98] and in the next its coordinates are [0, 0, 0.01], this atom will have moved 0.03*c, but if we only subtract the positions, we would get a displacement vector of [0, 0, -0.97]. Therefore, we can correct for this by adding or subtracting 1 from the value. To do this in a more simple way, we can just round that displacement vector (to [0, 0, -1]) and subtract it from the original displacement vector, giving us the correct displacement vector of [0, 0, 0.03].

[mjwen]

Thanks @sivonxay !

Yes, I hear you. We'd better keep backward compatibility as much as possible.

I guess the biggest issue for me is why keeping possibilities to use either frac_coords and displacement. Currently methods like __getitem__ are heavily overloaded. The returns depend heavily on the input type, and which mode (frac_coods or displacement) this class is in. I am imagining using only one of them, and then possibly a separate utility function to convert between the two. Hopefully this will significantly simplify the code. I guess I did not know the use cases, and keeping both of them might be easier in some cases. But for backward compatibility stuff, I think we'd better stick to what it is now.

[mjwen]

@mkhorton @sivonxay I've finally got this cleaned up. I've introduced one backward incompatibility (see top).

In addition, I have two TODOs to remove some code to further simplify it, and would love your feedback.

• Remove the .copy() function. We can use copy.copy().
• Remove the mode to return frac_coords in __getitem__, simplifying it to avoid overloading

After we get these two fixed, it'll be good for you to review.

[mkhorton]

Thanks @mjwen, I'll take a look -- I think the change to frame_properties is very sensible.

[mkhorton]

Let me know when this is ready to merge @mjwen when the last action items are handled, otherwise looks good

[mjwen]

I hoped to get some feedback on whether we want to do the two action items, and was waiting for it. But looking at my comments above, it is very badly communicated. 😞Thanks for checking back @mkhorton !

I am leaning towards performing the two items, and if you also think so, I'll make the changes.

[mkhorton]

No problem, I mis-read too -- as long as copy.copy() works as expected, I would remove it. I'm also in favor of simplifying __getitems__, although which mode should be the default I'm less sure about.

[mjwen]

@mkhorton I handled the two items, and this is ready for merging.

[sivonxay]

These changes look good. Thanks for cleaning up this class, @mjwen!

[mkhorton]

Thanks both, will get this merged. Adding a note to myself to include a mention of this change on our compatibility page.

[mkhorton]

Sorry I let this sit before merging, and now we have some merge conflicts. Would it be ok to ask to resolve? It's a bit confusing because both branch names are named master or I would have resolved quickly myself, but just want to make sure.

[mjwen]

@mkhorton conflicts resolved. Should be good now.

[mkhorton]

Thanks @mjwen!

[mjwen]

I saw this GH action error, which may require a hot fix?

7s
[1](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:1)
Run pytest pymatgen --ignore=pymatgen/analysis --ignore=pymatgen/electronic_structure --ignore=pymatgen/symmetry --ignore=pymatgen/ext --ignore=pymatgen/command_line --cov=pymatgen
[9](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:10)

[10](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:11)
==================================== ERRORS ====================================
[11](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:12)
___________ ERROR collecting pymatgen/core/tests/test_trajectory.py ____________
[12](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:13)
pymatgen/core/tests/test_trajectory.py:8: in <module>
[13](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:14)
    from pymatgen.core.trajectory import Trajectory
[14](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:15)
pymatgen/core/trajectory.py:35: in <module>
[15](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:16)
    Vector3D = tuple[float, float, float]
[16](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:17)
E   TypeError: 'type' object is not subscriptable
[17](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:18)
!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
[18](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:19)
1 error in 6.55s
[19](https://github.com/materialsproject/pymatgen/runs/8083026804?check_suite_focus=true#step:4:20)
Error: Process completed with exit code 1.