markovmodel / PyEMMA

🚂 Python API for Emma's Markov Model Algorithms 🚂
http://pyemma.org
GNU Lesser General Public License v3.0
311 stars 119 forks source link

Refactor plots2d #1317

Closed cwehmeyer closed 6 years ago

cwehmeyer commented 6 years ago

This PR refactors the plots2d module:

codecov[bot] commented 6 years ago

Codecov Report

Merging #1317 into devel will increase coverage by 0.4%. The diff coverage is 95.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##            devel    #1317     +/-   ##
=========================================
+ Coverage   91.64%   92.05%   +0.4%     
=========================================
  Files         223      224      +1     
  Lines       24756    26095   +1339     
=========================================
+ Hits        22687    24021   +1334     
- Misses       2069     2074      +5
Impacted Files Coverage Δ
pyemma/plots/tests/test_plots2d.py 100% <100%> (ø) :arrow_up:
pyemma/plots/__init__.py 100% <100%> (ø) :arrow_up:
pyemma/plots/plots1d.py 88.09% <90%> (+9.52%) :arrow_up:
pyemma/plots/tests/test_plots1d.py 95.83% <93.33%> (-4.17%) :arrow_down:
pyemma/plots/plots2d.py 94.16% <94.82%> (-0.93%) :arrow_down:
pyemma/msm/estimators/maximum_likelihood_hmsm.py 83.48% <0%> (-3.13%) :arrow_down:
pyemma/coordinates/tests/util.py 97.4% <0%> (-2.6%) :arrow_down:
pyemma/coordinates/data/sources_merger.py 93.65% <0%> (-1.35%) :arrow_down:
pyemma/util/contexts.py 83.82% <0%> (-1.18%) :arrow_down:
... and 23 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 f412fa8...4b66310. Read the comment docs.

marscher commented 6 years ago

Do we need to conserve the sequence of named parameters in legacy mode?

strictly speaking yes, because if somebody put in the parameters in the previous order, you would break the intended usage of the parameter.

cwehmeyer commented 6 years ago

@thempel please have a look at test_feature_histograms_nowarning in TestPlots1d.

thempel commented 6 years ago

Got it. This test was too unspecific and pretty much redundant (checked if no warning is raised for this particular function). Also, it did not show the raised warning. I removed the test and put some more meaningful ones there instead. Anyway, probably the failure was caused by this warning. Don't know if it's important, but just for the record.

RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (matplotlib.pyplot.figure) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam figure.max_open_warning). max_open_warning, RuntimeWarning)

thempel commented 6 years ago

For plot_density(), I get different behavior if using logscale or not. Logscale produces a discrete colormap which is otherwise continuous. Was that intended?

thempel commented 6 years ago

@cwehmeyer we discussed that it is potentially useful to have access to colormap or mappable. Since I found it a bit inconsistent to return either the colormap or the mappable, depending on the kwargs, they are now written into a dictionary and returned together. Otherwise, larger customized plots might break down if somebody chooses to alter some of the plot options. I'm not completely happy with it though; if you have a better idea feel free to implement it.

thempel commented 6 years ago

Thx @cwehmeyer. Can you add a line about the kwargs (i.e. what function they are passed to) to the docstrings?

cwehmeyer commented 6 years ago

@thempel: done.