<p dir="auto">Hi,</p>
<p dir="auto">I think there are some things in <a href="https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/hmm.py">https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/hmm.py</a> that could be improved.</p>
<h2 dir="auto">_BaseHMM.algorithm handling</h2>
<ul dir="auto">
<li>_BaseHMM.decode has an 'algorithm' argument that <em>doesn't</em> affect decoding algorithm if algorithm is already set in constructor OR if _BaseHMM.fit was called.</li>
<li>Getting/setting of an algorithm in _BaseHMM code often doesn't use property (there are direct accesses to _algorithm.</li>
<li>It is not possible to change a list of supported algorithms because decoder_algorithms is not an attribute of a class (and it is an immutable tuple), so the purpose of this variable is not clear.</li>
</ul>
<p dir="auto">Ideas:</p>
<ul dir="auto">
<li>Make decode_viterbi and decode_map methods public OR make 'algorithm' argument override default algorithm (this would require changing this argument to None by default).<br>
I'd prefer making decode_viterbi and decode_map public because flags that totally change what function does are bad API (see e.g. <a href="http://mail.python.org/pipermail/python-dev/2005-August/055907.html" rel="nofollow">http://mail.python.org/pipermail/python-dev/2005-August/055907.html</a>). Is it correct that "decode" is required in first place because of grid search, etc.?</li>
<li>always use a property to access 'algorithm' to make code cleaner;</li>
<li>kill 'decoder_algorithms' (and use simple ifs) OR make it class attribute of _BaseHMM.</li>
</ul>
<p dir="auto">I don't understand why _BaseHMM.fit sets algorithm.</p>
<h2 dir="auto">Docstrings</h2>
<ul dir="auto">
<li>_BaseHMM._decode_map docstring tells: "Find most likely state sequence corresponding to <code class="notranslate">obs</code>". In my understanding this is a bit misleading because (unlike _BaseHMM._decode_viterbi) likelihood of individual states is maximized, not the likelihood of a sequence.</li>
<li><code class="notranslate">params</code> and <code class="notranslate">init_params</code> are not properly documented. For example, for MultinomialHMM documented 'c' is not valid, and the most interesting 'e' is not documented;</li>
<li>module docstring tells us the module is to be removed in scikit-learn 0.11, but it is 0.14dev and it was not removed - what are the plans for sklearn.hmm?</li>
</ul>
<p dir="auto">Ideas: fix this.</p>
<h2 dir="auto">fit method</h2>
<ul dir="auto">
<li>There is no supervised <code class="notranslate">fit</code> variant for MultinomialHMM.</li>
<li>'obs' parameter name is quite obscure because it means different things in <code class="notranslate">fit</code> vs all other methods: it should be a list of sequences for <code class="notranslate">fit</code> and an individual sequence in other methods.</li>
<li>I don't get a note in _BaseHMM.fit docstring: _BaseHMM shouldn't care about covars_prior, and there is no pruning in current implementation.</li>
</ul>
<p dir="auto">Ideas:</p>
<ul dir="auto">
<li>change _BaseHMM.fit signature to <code class="notranslate">def fit(self, X, y=None)</code></li>
</ul>
<p dir="auto">My knowledge is lacking and I'm clueless about GaussianHMM and GMMHMM - does supervised learning make sense for them?</p>
<h2 dir="auto">Normalization</h2>
<ul dir="auto">
<li>I think there are cases when "hard" zeros in transition and emission matrices are wanted, and there are cases when matrices should be sparse, but sklearn.hmm uncomditionally adds adds EPS to each element of transmat and emissionprob, and this could be an issue.</li>
</ul>
<p dir="auto">I don't know if normalization is a real issue - maybe there are better solutions for sparse problems (like n-gram POS tagging with |tagset| &gt; 1000) - but sklearn.hmm is currently unusable for such problems at least because of normalization.</p>
<h1 dir="auto">misc</h1>
<ul dir="auto">
<li>I think default _compute_log_likelihood and _generate_sample_from_state implementations should raise NotImplementedError instead of returning None.</li>
<li>tiny cosmetic changes to _hmmc.pyx: <a class="commit-link" data-hovercard-type="commit" data-hovercard-url="https://github.com/kmike/scikit-learn/commit/ae0c4aba04b1ea6d73968991aae3d855a51daa58/hovercard" href="https://github.com/kmike/scikit-learn/commit/ae0c4aba04b1ea6d73968991aae3d855a51daa58">kmike@<tt>ae0c4ab</tt></a> (xrange -&gt; range; unneeded dashes are removed)</li>
</ul>
<hr>
<p dir="auto">I'm looking for a feedback (and would be happy to submit a pull request with fixes to some of these issues if this make sense).</p>
<h4 dir="auto">Describe the workflow you want to enable</h4>
<p dir="auto">This is possibly a duplicate of <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="135590071" data-permission-text="Title is private" data-url="https://github.com/scikit-learn/scikit-learn/issues/6424" data-hovercard-type="issue" data-hovercard-url="/scikit-learn/scikit-learn/issues/6424/hovercard" href="https://github.com/scikit-learn/scikit-learn/issues/6424">#6424</a>. I'd love to do a pipeline.fit(X) with a pandas dataframe with named columns, then a pipeline.get_feature_names() as input into an eli5 explainer, with scikit-learn being smart enough to call get_feature_names() for those transformers it makes sense to do so (OneHotEncoder, SelectFromModel, etc.)</p>
<h4 dir="auto">Describe your proposed solution</h4>
<p dir="auto">I'm not sure what a good solution might be -- one start might be to implement get_feature_names() for all transformers and return just the input column names if it doesn't change them, as would be the case for a Binarizer, for example.</p>
<h4 dir="auto">Describe alternatives you've considered, if relevant</h4>
<h4 dir="auto">Additional context</h4>