Error in degeneracy checking and PT-TEMPO

[piperfw]

PR #113 introduces an option in TEMPO computations to use degeneracies of the bath coupling operator eigensystem to reduce the dimension of the bath tensors used.

Testing indicated that differences of the order of at least 1e-4 can arise in the state calculated using this option versus not using it, at least in the presence of large degeneracies (an example is provided in tests/physics/large_degeneracy_test.py; this test is skipped in the current build).

If we ever intend to make this default behaviour, we should establish that this is not significant error e.g. by making comparisons using an exactly solvable model.

A related question is whether the same degeneracy checking can be used for PT-TEMPO.

[ewenlawrence]

Hi @piperfw and @gefux,

I've got a fork that implements PT-TEMPO degeneracy checking (OQuPY fork on my user). I've added a physics test as well that checking the same problem as the TEMPO degeneracy due to the similarities. I think as the general idea is the same between TEMPO and PT-TEMPO degeneracy checking, so I'm not sure if the same number of tests are required for PT-TEMPO. One possible test that I can think of is to test against the TEMPO unique=True result, as these should be identical up to numerical error.

One thing to note is the code currently fails on style because of too many instance attributes for PtTempoBackend (22/20), although I've only added 3 attributes (the same as TempoBackened) that all are rather necessary, so this maybe is needing a tidy up of the rest of the attributes. What are your opinions of this?

As for the large degeneracy tests, Peter suggested to use epsrel=0 to reduce the numerical inaccuracies between the two approaches, which gives agreement up to 10th decimal place it seems. Although I'm not sure this fully resolves the issue of testing, so I have not committed that change yet. The more involved plan is to do a spin 1 independent boson model, which should be analytically solvable, allowing a check between the exact result and the degeneracy checking. In addition I'm going to look at the convergence for degeneracy checking on and off for this exact model, to see if the issue of disagreement between the results is that one is converging faster than the other. Do you think these tests would be sufficient?

Let me know what you think about this, and I can make changes and send a pull request when ready

[piperfw]

Hi @ewenlawrence,
That's great! The pt comparison test you've added looks good and is probably sufficient (as you say). But testing PT vs non-PT TEMPO is also a nice idea - which I don't think we do elsewhere (?) - so by all means add that if you are inclined to.

Don't worry about the style failure. We can consider if we want to refactor the class later (indeed Gerald may be working on something), but for now disabling that check globally or locally seems fine. I can do this.

epsrel=0 result is reassuring. I did get the opportunity to have a word with Peter about this the other day. He suggested error with degeneracy checking might actually be less than without it [he also mentioned there were some quirks/inconsistencies in backend variables, could you comment on that @ewenlawrence?]. The test against an exact model you suggest should confirm that, and would be a valuable benchmark to have done. However for the present I think what you've done is already sufficient to put this into a PR and merge; you've been very fastidious here, thank you.

I'd be happy to field the PR with @gefux for a final sign off if you set it up. There's a backlog of PRs at the moment but hopefully will clear soon. Moreover I don't think the changes you have made will conflict with anything.

[ewenlawrence]

Hi @piperfw,

Thanks for the reply! I've gone ahead and added the pylint ignore for instance attributes and sent the Pull request through #129.

Yeah we do hope that the less numerical computations required due to the smaller bond dimension should lead to a higher numerical accuracy. I will get the test on an exact model working and setup another pull request when the behaviour is confirmed.

As for the comments of the quirks of the backend, the main part is similar to the tempo backend that the naming convection is not clear or documented. I think during a refactoring of the backend maybe starting from north and going clockwise for the indexes of the tensor would be the most clear. I found when three or two leg tensors where being made, it was quite hard to tell from the code which direction is which. I think either explicitly giving each tensor leg a direction or commenting above each different tensor the directions, would make developing the backend much easier.

[gefux]

Hi @ewenlawrence. Thank you for this. I'll comment on your code in the PR.

Also thank you for the feedback on the backend. I agree that things are not ideal. Concerning tensor indices I'd like to mention that there are two levels that should be strictly separated: 1] the input and output of some oqupy functionality and 2] the tensor network computation behind it. [1] is always in form of numpy arrays, where it should be clear and fixed which axis has which meaning. [2] the order of axis depends on the implementation --- in particular in tensornetwork there is no defined order because it uses the concept of nodes and edges. Clean code in google's tensornetwork only specifies the order of axis when importing from and exporting to a np.ndarray! This could have the advantage that the smart people at google can optimize the internal order of tensor networks, let it run on GPUs etc. That would be, if they'd be still working on this, which is not the case anymore .... so in practice this might not make a big difference, but the concept is still valid. :) This is maybe a point to discuss in #78

[piperfw]

#129 implements degeneracy checking for PT-TEMPO thanks to @ewenlawrence. Exact test and performance testing may follow

[gefux]

Done with PR #130