Closed marscher closed 4 years ago
Merging #1366 into devel will increase coverage by
0.06%
. The diff coverage is92.2%
.
@@ Coverage Diff @@
## devel #1366 +/- ##
==========================================
+ Coverage 91.49% 91.55% +0.06%
==========================================
Files 229 230 +1
Lines 25315 25492 +177
==========================================
+ Hits 23161 23339 +178
+ Misses 2154 2153 -1
Impacted Files | Coverage Δ | |
---|---|---|
pyemma/coordinates/transform/__init__.py | 100% <ø> (ø) |
:arrow_up: |
pyemma/coordinates/transform/vamp.py | 92.28% <ø> (-0.03%) |
:arrow_down: |
pyemma/msm/estimators/_dtraj_stats.py | 85.29% <ø> (ø) |
:arrow_up: |
pyemma/_ext/variational/estimators/moments.py | 91.09% <ø> (ø) |
:arrow_up: |
pyemma/coordinates/data/_base/transformer.py | 91.93% <100%> (ø) |
:arrow_up: |
pyemma/_base/model.py | 89.28% <100%> (-1.08%) |
:arrow_down: |
pyemma/coordinates/tests/test_serialization.py | 98.35% <100%> (+0.18%) |
:arrow_up: |
pyemma/coordinates/api.py | 88.21% <100%> (ø) |
:arrow_up: |
pyemma/coordinates/data/_base/datasource.py | 90.48% <100%> (ø) |
:arrow_up: |
pyemma/_base/serialization/serialization.py | 82.1% <100%> (+1.86%) |
:arrow_up: |
... and 13 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 61cc7a5...8da4efa. Read the comment docs.
@fabian-paul I'd be delighted, if you could have a look at the changes I've made to TICA, especially the newly introduced rank handling in the dimension reduction. If you think this PR is too big, I could try to tidy/split it up. Thanks in advance!
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
:*(
In this PR we moved the handling the dimension and diagonalization from TICA to its model. This turned out to be good solution for the new VAMP estimator/model pair and therefore we should use it from now on for TICA and its children.
In VAMP the dim parameter can either take a fixed dimension (integer), a float for variance cutoff, or None to preserve all available ranks. To handle optional scaling of the singular/eigenvectors it has a 'scaling' parameter taking strings like 'kinetic_map' etc.
This approach should fix #1075, since it reduces the amount of input parameters and invalid combinations.
The default behavior is not changed, it still uses kinetic map scaling and preservation of 95% of kinetic variance.
The changes have also been applied for NystroemTICA. This avoids hard to maintain copy paste code, as this implementation copied over almost everything from TICA except for the low-rank, column selection section.
Already serialized TICA objects are future proof as well, as these are transformed during loading to fit into the new class layout.