Improve jump specification criteria

[v1kko]

• Specification of site radius in angstr

  sites = SitesData(structure=structure,
                    trajectory=vasp_traj,
                    floating_specie='Li',
                    site_radius=1, # This can be left out
                    site_inner_fraction=0.5)

• Addition of Jumps class

  jumps = Jumps(transitions=transitions,
                sites=sites,
                conversion_method=<Function to convert transitions to Jumps>
                minimal_residence=4)
  

• transitions and jumps are now pandas dataframes
• A generic conversion method is supplied: _generic_transitions_to_jumps which is used as default, and can be used to write your own converter.
• The dataframe for transitions looks like:

      atom index  start site  destination site  time
0              0          94                -1   202
1              0          -1                94   204
2              0          94                -1   208
3              0          -1                94   219
4              0          94                -1   224
...          ...         ...               ...   ...
2100          47          47                -1  3421
2101          47          -1                47  3440
2102          47          47                -1  3665
2103          47          -1                55  3677
2104          47          55                -1  3691

[2105 rows x 4 columns]

• The dataframe for jumps looks like:

     atom index  start site  destination site  start time  stop time
0             0          94                 0         282        284
1             0           0                94         385        442
2             0          94                 0         704        728
3             0           0                94         920        928
4             0          94                 0        1003       1006
..          ...         ...               ...         ...        ...
457          47          47                69         961        964
458          47          69                47        1014       1017
459          47          47                39        1212       1231
460          47          39                47        2751       2771
461          47          47                55        3665       3678

• Specification of minimal residence time to count as jump
• inner site radius specification
• write tests
• write docs

[v1kko]

@AstyLavrinenko, @stefsmeets a notebook with the changes can be found here: https://gemdat.readthedocs.io/en/jumps_update/notebooks/jumps/

[stefsmeets]

Hi @v1kko , thanks for this. I understand the goal of the PR is to split off the jumps related code into its own class. Changing events into a dataframe will be super useful going forward.

To my taste this PR is too large, so I wasn't able to consider the changes as a whole. Some of the code became a bit too complex for me to fully grasp what is going on. So I just left some comments as I went through the code.

I have 2 major points of concern:

1. The changes are useful, and decoupling of Jumps is a nice step forward. My concern is the complexity of the code and api, which has also taken a leap. I think it would be useful to go through the code and make sure the docstrings are up to date, that we avoid returning tuples (a code smell, if you ask me), and to refactor a bit to make sure that the api still pleasant to work with.

2. The test data for the integration test are completely different. It's not clear to me exactly from the PR what has changed or why this is. This makes it difficult to verify the correctness of the code. Some small differences can be expected, but the results here are completely different. To me it indecates a problem with the code if we cannot reproduce the results from our test data.

[v1kko]

Hi @v1kko , thanks for this. I understand the goal of the PR is to split off the jumps related code into its own class. Changing events into a dataframe will be super useful going forward.

To my taste this PR is too large, so I wasn't able to consider the changes as a whole. Some of the code became a bit too complex for me to fully grasp what is going on. So I just left some comments as I went through the code.

I have 2 major points of concern:

1. The changes are useful, and decoupling of `Jumps` is a nice step forward. My concern is the complexity of the code and api, which has also taken a leap. I think it would be useful to go through the code and make sure the docstrings are up to date, that we avoid returning tuples (a code smell, if you ask me), and to refactor a bit to make sure that the api still pleasant to work with.

2. The test data for the integration test are completely different. It's not clear to me exactly from the PR what has changed or why this is. This makes it difficult to verify the correctness of the code. Some small differences can be expected, but the results here are completely different. To me it indecates a problem with the code if we cannot reproduce the results from our test data.

Thanks for looking.

I don't think it can be helped that this PR is so large, introducing "configurable" jumps unfortunately touches almost everything, but suggestions are welcome.

I don't think returning tuples is always bad, but I can fix it here.

Number 2 is because most of these numbers did not make sense (especially the calculation of solo and collective jumps). This is now improved, so this change is expected, and discussed with Alexandros. I should have mentioned it before. If you look at the figures for jumps for example those are still similar.

[stefsmeets]

I don't think returning tuples is always bad, but I can fix it here.

My gripe is that it leads to unmaintainable code. Adding another item breaks all code downstream (Similar to having functions with many positional arguments). I find that in most cases it is easier in the long run to return a dataclass, namedtuple or dict, so that variables can be accessed by name, rather than by index. Usually, I find that this leads to a better design.

Number 2 is because most of these numbers did not make sense (especially the calculation of solo and collective jumps). This is now improved, so this change is expected, and discussed with Alexandros. I should have mentioned it before. If you look at the figures for jumps for example those are still similar.

I see now that the 450 comes back in other places. 👍 I wasn't aware that there were some issues with the previous implementation. I'm still curious what you think about all those other values being so far off.