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

latexify hmm_poisson_changepoint.ipynb #129

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TakshPanchal
Copy link
Contributor

Description

Figure Number

Figure 29.6, 29.7 and 29.8

Figures

Figure 29.6

Before PR After PR
29.6B 29.6A

Figure 29.7

Before PR After PR
Screenshot from 2022-07-13 15-31-57 Screenshot from 2022-07-13 15-31-41

Figure 29.8

Before PR After PR
Screenshot from 2022-07-13 15-37-24 Screenshot from 2022-07-13 15-37-08

Issue

#113

Steps

  • Latexify

Checklist:

  • Performed a self-review of the code
  • Tested on Google Colab.

Potential problems/Important remarks

When I tried to add legends in fig 29.7, it was too cumbersome. Should I add a legend to fig 29.7 figures?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@karm-patel
Copy link
Contributor

karm-patel commented Jul 13, 2022

Regarding the legend in Fig 29.7

  1. In the case of the same legend in every plot, we usually add a legend in only one plot, the ideally first plot should have a legend, but if the first plot doesn't have enough space for the legend, then we can choose any plot which has more space.
  2. In case if legend's boundary is overlapping with plot you can use ax.legend(..., frameon=False)

@karm-patel
Copy link
Contributor

Fig 29.6

  1. You can put the title of the 2nd subplot in subcaption like (b) Inferred
  2. Because we need the same ylim for both the subplots, the first comment would resolve this issue.

@karm-patel
Copy link
Contributor

@TakshPanchal , Other things look good to me, you can create a PR in the main repo after resolving the above comments.

@TakshPanchal
Copy link
Contributor Author

Removing the frame still didn't work out so I set fontsize to xx-small
image

@TakshPanchal
Copy link
Contributor Author

Fig 29.6

image

@TakshPanchal
Copy link
Contributor Author

  • You can put the title of the 2nd subplot in subcaption like (b) Inferred

It will be the change in the latex file of the book right? 🤔

@karm-patel
Copy link
Contributor

  • You can put the title of the 2nd subplot in subcaption like (b) Inferred

It will be the change in the latex file of the book right? thinking

Yes right, I'll do that. your SS will remind me to edit latex :)

@karm-patel
Copy link
Contributor

Removing the frame still didn't work out so I set fontsize to xx-small image

Try fontsize in numbers always, so we can get an idea, start with 9 and then decrease, ideally 7 should be minimum
Here you can apply some hacks, like add "\n" - plt.plot(..., label="inferred\nrate")

@karm-patel
Copy link
Contributor

And I prefer to add a legend in the first subplot, otherwise, it would not quickly visible.

@TakshPanchal
Copy link
Contributor Author

Okay. understood.

@TakshPanchal
Copy link
Contributor Author

How's this looking?
image

@karm-patel
Copy link
Contributor

How's this looking? image

Ummm, looks a little crowdy,
Maybe add a legend in 4th plot. Because 4th plot is also the first plot. And please tell me fontsize also

@karm-patel
Copy link
Contributor

One more thing, Now I think you don't need to use reverse style legend labels, right?

@karm-patel
Copy link
Contributor

@TakshPanchal
try out this comment also :)
image

@karm-patel
Copy link
Contributor

Great @TakshPanchal , Go ahead with PR in the main repo.

@TakshPanchal
Copy link
Contributor Author

Done :) and fontsize is 7
Screenshot from 2022-07-14 19-32-19

@karm-patel
Copy link
Contributor

@TakshPanchal Wait,

Do you know the reason why this low spike is there? Previously it was not there
image

@@ -0,0 +1,1013 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Line #1.    rng_key, rng_normal = jax.random.split(rng_key)

may be you can try changing rng_key to reproduce the results.


Reply via ReviewNB

@karm-patel
Copy link
Contributor

@TakshPanchal, don't spend much time to reproduce 2nd plot, just try some rng_keys, otherwise, you can ask this to Dr. Kevin while creating PR, who knows this spike is obvious.

@TakshPanchal
Copy link
Contributor Author

Okay got it.

@TakshPanchal
Copy link
Contributor Author

I tried some values of seeds and rng_keys but no change in spike.

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.

None yet

2 participants