[DOC] Migrating Examples to Notebooks: plot_em.ipynb EM estimation of model parameters with a Kalman Filter #164
Conversation
|
I have added the matplotlib in |
|
@phoeenniixx can you please review this? |
There was a problem hiding this comment.
Great! I think this is good!
There are a number of points that could be improved, but these issues are already there with the original example and are not in the scope of a simple migration, I would do this in a second step.
Some things that should be fixed:
- TeX formulae translated are not exactly correctly transferred, e.g., "timesteps" is displayed at a different index level
- I would also not use top level headers for every cell - can you turn these into simple markdown cells for now?
…ed the working for better explaination
Hey @fkiraly, as per these 2 points, I have attempted to fix the issues. I have removed the top level headers and replaced with normal sized text. Also, Latex equations in the markdown renders well. |
There was a problem hiding this comment.
This is great, thanks!
I think it is a faithful translation, so it is good as migrating gthe example.
Next, may I suggest:
- proceed with further notebooks
- there are some differences in expected style in how one would expect notebooks in jupyter, from a commented python file. I will comment below.
Suggestions for improvements to this notebook (separate PR):
- the first paragraph describes the figure which appears much further down. I suggest you move the description of the figure above the figure.
- clarity can be improved for statements like "estimtate the hidden state using observations" - try to link the variable names to the statements in English. You can do this by modifying the statement, or by adding another sentence, in direct reference to th epython objects that are manipulated or created in the following cell (this comment applies to all cells)
|
The tests do not seem to run - do you know why, @PythonHacker24? |
|
@fkiraly I have added another test to verify all the notebooks that will be uploaded, but it should not stop any tests from happening. On my repository, it started running (on the old PR), and I cross-confirmed that the tests are running and successful for the notebooks. |
|
@fkiraly Any updates on this? I need to start working on other notebook migrations and move forward to work on implementing EnFK after that (that's were real fun is gonna start! can't wait to work on it ;) ) |
.github/workflows/run-ci.yml
Outdated
| @@ -100,3 +100,32 @@ jobs: | |||
| - name: Run Unit Tests | |||
| run: pytest pykalman/tests/ | |||
| shell: bash | |||
|
|
|||
| test-notebooks: | |||
There was a problem hiding this comment.
Not sure but the issue could be the indentation?
are test-notebooks and build at the same level? (I think they should be or am I missing something here?)
There was a problem hiding this comment.
Done, fixed the indentations, good catch 🙌
doc/source/conf.py
Outdated
| "sphinx.ext.todo", | ||
| "sphinx.ext.imgmath", | ||
| "numpydoc", | ||
| "nbsphinx", | ||
| "sphinx.ext.mathjax", |
There was a problem hiding this comment.
just a minor non-blocking comment - I think these should be in alphabetic order?
I mean sphinx.ext.mathjax should be after sphinx.ext.autosummary?
Examples to Notebook Migration
As mentioned in #129 and following a brief discussion with @fkiraly on Discord, I am initiating the process of migrating examples in the documentation from Python files to a notebook.
If I understand this one file, I can work on migrating all the examples into notebook examples and adding them into the docs correctly.
I would greatly appreciate your input on this, and I look forward to receiving it as soon as possible.