markovmodel / PyEMMA

πŸš‚ Python API for Emma's Markov Model Algorithms πŸš‚
http://pyemma.org
GNU Lesser General Public License v3.0
307 stars 118 forks source link

Fixes #1443 #1449

Closed fabian-paul closed 4 years ago

franknoe commented 4 years ago

Looks good to me superficially - but I have not followed the state discussion in detail.

fabian-paul commented 4 years ago

I'm a bit disconcerted by the fact that we have not spotted to problem before.

codecov[bot] commented 4 years ago

Codecov Report

Merging #1449 into devel will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel    #1449      +/-   ##
==========================================
+ Coverage   91.83%   91.84%   +<.01%     
==========================================
  Files         229      229              
  Lines       26138    26139       +1     
==========================================
+ Hits        24004    24007       +3     
+ Misses       2134     2132       -2
Impacted Files Coverage Ξ”
pyemma/thermo/estimators/TRAM_estimator.py 76.43% <100%> (+0.3%) :arrow_up:
pyemma/thermo/estimators/DTRAM_estimator.py 96% <100%> (+0.1%) :arrow_up:
pyemma/msm/estimators/_dtraj_stats.py 83.01% <0%> (+0.3%) :arrow_up:
pyemma/_ext/sklearn/base.py 62.59% <0%> (+0.76%) :arrow_up:

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 e4c6e84...5e7ebfd. Read the comment docs.

clonker commented 4 years ago

Looks good to me, per model definition pi only acts on the active set so it also has to be restricted if the energies live on the full state space. Only thing that might be worth adding is a small test case in which a state gets dropped and we check that the shape of pi matches the number of active states. Otherwise I am also ok with merging this @fabian-paul @franknoe .

fabian-paul commented 4 years ago

Here are tests.

clonker commented 4 years ago

Cool thanks, will merge once CI gives green light!