PT-TEMPO degeneracy checking

[ewenlawrence]

This pull request aims to resolve most of issue #115, by implementing degeneracy checking for PT-TEMPO. Further work needs to be done to verify the failing test, although it does pass with epsrel=0 up to 10 decimal places, a later pull request should confirm the accuracy of the approach before making this default behaviour.

I have left unticked what I think are un relevant checks, otherwise all checks done. Note there is an ignored pylint test for too many instance attributes, as this is related to a general refactor of the PtTempoBackend class that will be resolved during tidy up of classes.

Pull Request Check List

• The contribution has been discussed and agreed on in the Issue section.
• Code contributions do its best to follow the zen of python.
• The automated test are all positive:
  • tox -e py36 (to run pytest) the code tests.
  • tox -e style (to run pylint) the code style tests.
  • tox -e docs (to run sphinx) generate the documentation.
• Added test for changed/added code.
• API code contributions include input checks (defensive code).
• API code contributions include helpful error messages.
• The documentation has been updated:
  • docstring for all new functions/methods/classes/modules.
  • consistent style of all docstrings.
  • for new modules: /docs/pages/modules.rst has been updated.
  • for api contributions: /docs/pages/api.rst has been updated.
  • for api contributions: tutorials and examples have been updated.

[gefux]

Great. It is nice to see that not much needs to be changed for the PT-TEMPO degeneracy checking, given your implementation for TEMPO!

I think this is almost ready to merge. I have only one request concerning the parameter count in the backend:
The degeneracy is either not applied or both (east-west and south-north) are used. Hence, I think the right parameter to pass is just one, namely: 'degeneracy_maps' which is a pair of ndarrays and defaults to "None" when it is not used. Does that make sense? I think this would also keep you below the (current) parameter limit for linter.

[piperfw]

Hi both,
Gerald's suggestion was clear to me and I realised it probably should be done in a consistent way across both Tempo and PtTempo backends, so I went ahead and did that. This involved changing the signatures of BaseTempoBackend, TempoBackend, MeanFieldTempoBackend constructors, as well as the influence_matrix (or _influence) functions. Basically, anything user-facing uses a unique=True argument to turn degeneracy checking on, and everything internal uses a degeneracy_maps which is either a list of two arrays for the degeneracy checking, or None (in which case no degeneracy checks are made). As these changes are all internal, the operation of the tutorials etc. is unchanged.

There was some loss of type 'safety' checking, but mainly in a simplfying way (before we were checking the lists were simultaneously not None etc.). See also #107

@ewenlawrence if you check you're happy with what I've done here, let me know and I'll get this merged - I've updated the docstrings and don't think any other documentation needs changing.

For now I've left the tutorial texts as is. I think if the exact comparison is made (or possibly before then) we can have unique=True the default and change the text to point out unique=False may be specified to disable this feature (note for a PT-TEMPO calculation this must be done at the time of creating the PT, not calculating the dynamics).

On a related note I added a couple of comments on overleaf regarding the degeneracy checking. I guess we will be able to cite the JCP in the tutorial/docs regarding this feature. My thesis also has a brief discussion if needed.

[ewenlawrence]

Hi both,

The proposed change looks like a really elegant implementation, and simplifies the code nicely. @piperfw thanks very much for implementing that change, I am happy with the changes.

I'll be trying the exact solution this week, so I will try and get that back to you both as soon as it's ready.

Thanks as well piper for the comments on the overleaf, I'll see those on the next pass through the section

[piperfw]

Great @ewenlawrence, I'm just battling a huge rebase for the multi-time correlation PR then yours is next!

No problem, I guess that section should be moved earlier. Also, I remembered Danius' thesis also has a discussion of degenerate couplings, which you may or may not decide to cite (see my thesis bottom p. 52).

Look forward to hearing about the exact comparison in due course

[piperfw]

That's finalised @ewenlawrence, thanks again for the work on this.