Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new section in HMM tutorial #508

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JasonPekos
Copy link
Member

This adds a section to the HMMs tutorial that has a much faster and more concise version of the model (four lines!) using HiddenMarkovModels.jl.

There's a subsection after that which shows how to recover the state estimates with the Viterbi algorithm.


This closes:

Copy link
Contributor

Preview the changes: https://turinglang.org/docs/pr-previews/508
Please avoid using the search feature and navigation bar in PR previews!

@yebai
Copy link
Member

yebai commented Sep 25, 2024

Thanks @JasonPekos. It looks good!

@yebai yebai requested a review from penelopeysm September 25, 2024 21:40
tutorials/04-hidden-markov-model/index.qmd Outdated Show resolved Hide resolved
tutorials/04-hidden-markov-model/index.qmd Outdated Show resolved Hide resolved
tutorials/04-hidden-markov-model/index.qmd Outdated Show resolved Hide resolved
Comment on lines +267 to +271
#| code-fold: true
#| code-summary: "HMM Plotting Functions"

p = plot(xlim=(0, 30), ylim=(-1, 5), size=(500, 250))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```{julia}
#| code-fold: true
#| code-summary: "HMM Plotting Functions"
p = plot(xlim=(0, 30), ylim=(-1, 5), size=(500, 250))
```{julia}
p = plot(xlim=(0, 30), ylim=(-1, 5), size=(500, 250))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like how this vector — which contains no information not present in the plot, and which is not part of anyones actual workflow (in practice, people just import data via csvs etc.) — takes up so much space on the page.

that is why I originally introduced the folder code blocks (I agree that the other use especially is excessive, but I am more torn here).

I am considering doing something like

y = [i for i in vcat(fill(1.0, 6), fill(2.0, 6), fill(3.0, 7), fill(2.0, 4), fill(1.0, 7))]

but ofc this is potentially more confusing — thoughts?

Copy link
Member

@penelopeysm penelopeysm Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the vector isn't this code block though – it's the one above. I do see your point though! I'm sorry if I sound too conservative here about code folding :) It's mainly because I'd like to preserve a consistent user experience across the docs. But if there are good places we can apply folding to in the docs I'm super happy to start using it.

For this particular vector, would this work maybe?

y = [fill(1.0, 6)..., fill(2.0, 6)..., fill(3.0, 7)..., fill(2.0, 4)..., fill(1.0, 7)...]

The other alternative would just be to put multiple values on one line:

y = [
    1.0, 1.0, 1.0, ...,
    ...,
]

though I recognise JuliaFormatter doesn't like that. Although we aren't explicitly checking code style in this repo, there is a page outlining the style guide so I guess we should try to practise what we preach 😄

Comment on lines +206 to +209
#| output: false
using HiddenMarkovModels
using FillArrays
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do setprogress!(false) instead of using Quarto's output: false?

tutorials/04-hidden-markov-model/index.qmd Show resolved Hide resolved
Comment on lines +243 to +245
#| output: false
@model function BayesHmmRecover(y, K, IncludeGenerated = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above – if we do setprogress!(false) above can we enable the output as usual?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could, but do we want to? I assume most users will want the progress updates in their REPL when working through the tutorial, but they just aren't a good fit for the published quarto format.

As long as we are not actually recommending users use setprogress!(false) (which I think we aren't?), it seems appropriate to just disable this in the chunk options for the rendered docs.

Copy link
Member

@penelopeysm penelopeysm Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact (I totally missed this earlier) but there is already a call to setprogress!(false) earlier in the page (which is hidden). Many other pages do call setprogress!(false) so it wouldn't be too out of place here, so I feel like it should be alright to just remove the output: false lines.

I'd be happy to add a clarification about setprogress! on the get started page in a separate PR :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(By the way, I'm assuming that the output you wanted to suppress was indeed the progress logging. Do correct me if that's not the case)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants