Opened 5 years ago

Last modified 4 years ago

#22750 needs_review enhancement

Implement Generalized Hidden Markov Models

Reported by: msaaltink Owned by:
Priority: minor Milestone: sage-8.4
Component: statistics Keywords:
Cc: Merged in:
Authors: Mark Saaltink Reviewers:
Report Upstream: N/A Work issues:
Branch: public/22750_implement_generalized_hidden_markov_models (Commits, GitHub, GitLab) Commit: 4159a5c011d076c60885fa0cdb2575ecc9c58959
Dependencies: Stopgaps:

Status badges

Description (last modified by msaaltink)

GHMMs as defined by Upper have algorithms for equivalence and minimization, so may be useful addition to the Hidden Markov Model suite.

Change History (9)

comment:1 Changed 5 years ago by msaaltink

  • Branch set to u/msaaltink/implement_generalized_hidden_markov_models

comment:2 Changed 5 years ago by msaaltink

  • Authors set to Mark Saaltink
  • Commit set to 2e7d90eb48f21e261bdb09232fcc73d61d8c9510
  • Component changed from PLEASE CHANGE to statistics
  • Description modified (diff)
  • Priority changed from major to minor
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

New commits:

2e7d90etrac 22750: implement generalized hidden markov models: basic functions and documentation.

comment:3 Changed 5 years ago by tscrim

  • Status changed from needs_review to needs_work

I don't really know the math, but I have some quick comments about the code:

  • Sage uses """ instead of ''' for docstrings.
  • Your import of from sage.stats.hmm import hmm brings in numpy, so I would also lazily import your module.
  • You have some Python2 specific syntax for an error message.
  • You can do
    -G = copy(g)
    -f = G.relabel(return_map=True)
    f,G = g.relabel(inplace=False, return_map=True)
    
  • Short error messages should not start with a capital letter.
  • INPUT: blocks should not be indented.
  • 1 line descriptions should be on a separate line (e.g., not as """Short description.).
  • Not all methods have doctests.
  • Use not (self == other) instead of not self.__eq__(other).

comment:4 Changed 5 years ago by msaaltink

Good suggestions. On the issue of g.relabel, I wanted to do that, but the documentation says

   If "return_map" is "True" a dictionary representing the relabelling
   map is returned (incompatible with "inplace==False").

and I'm worried that this is not going to work in all cases. I'll have a closer look at relabeling; perhaps the documentation is wrong.

comment:5 Changed 5 years ago by tscrim

Ah, I missed that. So then what you have is okay.

comment:6 Changed 5 years ago by git

  • Commit changed from 2e7d90eb48f21e261bdb09232fcc73d61d8c9510 to aa56687ef6ff0540ecb87570724f779e9bb79c83

Branch pushed to git repo; I updated commit sha1. New commits:

aa56687Made changes suggested by reviewer; mostly style issues

comment:7 Changed 5 years ago by msaaltink

  • Status changed from needs_work to needs_review

comment:8 Changed 4 years ago by gh-bryangingechen

  • Status changed from needs_review to needs_work

This patch no longer merges.

comment:9 Changed 4 years ago by gh-bryangingechen

  • Branch changed from u/msaaltink/implement_generalized_hidden_markov_models to public/22750_implement_generalized_hidden_markov_models
  • Commit changed from aa56687ef6ff0540ecb87570724f779e9bb79c83 to 4159a5c011d076c60885fa0cdb2575ecc9c58959
  • Milestone changed from sage-8.0 to sage-8.4
  • Status changed from needs_work to needs_review

I've fixed the merge issue in this commit and removed the dependence on the deprecated sage.matrix.matrix (see #24096).


New commits:

2e7d90etrac 22750: implement generalized hidden markov models: basic functions and documentation.
aa56687Made changes suggested by reviewer; mostly style issues
01fd208Merge branch 'u/msaaltink/implement_generalized_hidden_markov_models' of git://trac.sagemath.org/sage into 22750_ghmm
4159a5cDeal with deprecated sage.matrix.matrix (see #24096)
Note: See TracTickets for help on using tickets.