vincent-maillou / qttools

Quantum Transport Algorithms Toolbox
GNU General Public License v3.0
5 stars 1 forks source link

Refactoring of OBC interface to allow solution via NEVP #31

Closed vetschn closed 4 weeks ago

vetschn commented 2 months ago

I added Beyn's method and the spectral solver using it that @almaeder has been working on. I also added some better test cases for the OBC.

Some things to still do:

This should close #14 and some related issues.

codecov-commenter commented 2 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.86885% with 37 lines in your changes missing coverage. Please review.

Project coverage is 82.79%. Comparing base (386b046) to head (a3383fa). Report is 2 commits behind head on dev.

Files with missing lines Patch % Lines
src/qttools/obc/spectral.py 84.14% 13 Missing :warning:
src/qttools/obc/obc.py 75.00% 8 Missing :warning:
src/qttools/lyapunov/lyapunov.py 82.85% 6 Missing :warning:
src/qttools/lyapunov/doubling.py 87.50% 3 Missing :warning:
src/qttools/datastructures/dsbcoo.py 0.00% 2 Missing :warning:
src/qttools/lyapunov/spectral.py 88.23% 2 Missing :warning:
src/qttools/lyapunov/vectorize.py 87.50% 2 Missing :warning:
src/qttools/nevp/nevp.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #31 +/- ## ========================================== + Coverage 80.71% 82.79% +2.07% ========================================== Files 18 29 +11 Lines 809 1081 +272 ========================================== + Hits 653 895 +242 - Misses 156 186 +30 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vetschn commented 4 weeks ago

The failing test is due to the choice of parameters for Beyn. I tried to tune it but it still fails like 25% of time. We really would need to come up with a better way of constructing NEVP with pre-selected eigenvalues... :(

vetschn commented 4 weeks ago

I left a bunch of comments, in general it's surely ready to be merged. Let me know when you've reviewed them and if nothing big comes out let me know, and I'll accept the merge.

EDIT: Yes also I spotted that the coverage for this pull-request is quite low (68% against 78% of project). I know it's not the alpha-and-omega, what are your thoughts?

Thanks a lot for having a look, you caught several important things! Yes, coverage should be a bit higher. The Memoizers are currently untested and so is the x_ii_formula keyword in the Spectral solver. We'll see how much it improves when i add tests for those :+1: