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

Improvements to GP documentation #3027

Merged
merged 8 commits into from
Mar 9, 2022
Merged

Conversation

nipunbatra
Copy link
Contributor

  1. Added xlabel and ylabel to loss v/s iterations plots
  2. Added animation for fitting GP
  3. Added plot of inducing points
  4. Added animation for fitting inducing point GP
  5. Added example showing impact of lengthscale#

2. Added animation for fitting GP
3. Added plot of inducing points
4. Added animation for fitting inducing point GP
5. Added example showing impact of lengthscale#
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nipunbatra
Copy link
Contributor Author

This PR partially addresses: #3025 (the GP classification part in 3025 will be handled in a separate PR)

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 28, 2022

View / edit / reply to this conversation on ReviewNB

fehiepsi commented on 2022-02-28T16:11:32Z
----------------------------------------------------------------

Line #19.        locations.append(deepcopy(sgpr.Xu.data.numpy()))

Any reason for using deepcopy?


nipunbatra commented on 2022-02-28T23:09:43Z
----------------------------------------------------------------

I can try once again without deep copy. I remember my locations array without deep copy was having #iterations X sgpr.Xu.data for the last iteration only.

nipunbatra commented on 2022-03-01T05:22:33Z
----------------------------------------------------------------

I tried again without deepcopy and confirm that if I do not use deepcopy the animation will pick up only the last iteration sgpr.Xu.data. This does not happen for other variables, like kernel.*

fehiepsi commented on 2022-03-01T23:59:37Z
----------------------------------------------------------------

I think other variables are scalars after .item(), but Xu.data.numpy() is still an array that shares the same memory with Xu. Maybe Xu.data.numpy().copy() works?

@fehiepsi
Copy link
Member

Great improvements, thanks @nipunbatra! I just concern a bit about the large diff caused by the animations. Is there a way to simplify it?

Copy link
Contributor Author

I can try once again without deep copy. I remember my locations array without deep copy was having #iterations X sgpr.Xu.data for the last iteration only.


View entire conversation on ReviewNB

Copy link
Contributor Author

nipunbatra commented Feb 28, 2022

Thanks, I'm not fully sure if I understand the question.

Is it:

Can we reduce the code for animation?

  1. One way could be set code for animation as a collapsed cell? With Jupyter book this is possible, not sure possible with the docs as they currently are.
  2. Other way could be to remove the code and only have the animation GIF?
  3. I can try to look again to reduce the code size for animation, but I'm doubtful there is much scope there.

Or, Can we reduce the file size for animations?

  1. We can do this by reducing the number of frames used in the animation
  2. We can reduce the figure size for figures used in the animation
  3. We can save the animations as a GIF
  4. We can save the animations as a GIF at lower resolution

Please let me know what you think and any other suggestions as well!

Copy link
Contributor Author

I tried again without deepcopy and confirm that if I do not use deepcopy the animation will pick up only the last iteration supra.Xu.data. This does not happen for other variables, like kernel.*


View entire conversation on ReviewNB

Copy link
Member

fehiepsi commented Mar 1, 2022

I think other variables are scalars after .item(), but Xu.data.numpy() is still an array that shares the same memory with Xu. Maybe Xu.data.numpy().copy() works?


View entire conversation on ReviewNB

Copy link
Member

fehiepsi commented Mar 2, 2022

I meant that with animation, you are adding more than 200,000 lines to the notebook, which makes it heavy to load and render (took me a while to open ReviewNB for this notebook). With your suggestions, I think we can save the animation (using animation I guess) to some format like gif or mp4 but I'm not sure where to store them (maybe this folder is a good candidate).

@nipunbatra
Copy link
Contributor Author

Thanks @fehiepsi can you please mention the folder again? The link to the previous folder you have mentioned (https://github.com/pyro-ppl/pyro/pull/using%20animation%20I%20guess) seems broken to me.

I'll make these changes and revise the PR.

@fehiepsi
Copy link
Member

fehiepsi commented Mar 2, 2022

Oops, here is the folder that I mentioned: https://github.com/pyro-ppl/pyro/tree/dev/tutorial/source/_static/img

1. Adds a simple example for binary GP classification
2. Adds a simple example for multi-class GP classification using the
   Iris dataset
3. Removes deepcopy for inducing points location, instead use
   np.array.copy()
4. Changes the animations from HTML to GIFs (each GIF is 1-2 MBs)
5. Miscelleaneous trivial cleanup and Black formatting
@nipunbatra
Copy link
Contributor Author

Hi @fehiepsi
I have made the edits as per your inputs and request your further feedback to help merge this PR.

  1. Adds a simple example for binary GP classification
  2. Adds a simple example for multi-class GP classification using the
    Iris dataset
  3. Removes deepcopy for inducing points location, instead use
    np.array.copy()
  4. Changes the animations from HTML to GIFs (each GIF is 1-2 MBs)
  5. Miscelleaneous trivial cleanup and Black formatting

fehiepsi
fehiepsi previously approved these changes Mar 6, 2022
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Hi @nipunbatra, the new content looks great to me! Could you execute the output of the later cells?

@nipunbatra
Copy link
Contributor Author

Thanks, @fehiepsi I'll rerun the entire notebook and make a commit.

I also wanted to propose a couple of other additions. Please let me know if they look fine to be included. If yes, should I append them in this PR itself or create a new PR.

  1. minimal example on 1d step data showing deep kernel v/s RBF kernel
    Unknown

  2. similar to this example from tinyGP and GPyTorch, I can create a small example on Poisson likelihood

  3. in the classification example, I can additionally show with ARD (different lengthscale per dimension) and a Polynomial kernel in addition to the RBF (without ARD kernel) we're already showing.

@fehiepsi
Copy link
Member

fehiepsi commented Mar 7, 2022

Hi @nipunbatra, I think it is great to show applications of Pyro GPs those many examples, but I hope we can make the tutorial a bit concise. Illustrating deep kernel and Poisson likelihood seems unnecessary to me. For ARD, it would be nice to modify the example to illustrate it, rather than adding an additional one. Maybe for the current Binary classification, you can add RBF with an additional Martern kernel to illustrate that kernels can be combined. What do you think?

1. trivial example for deep kernel learning
2. combination of kernels
3. using ARD in DKL example
4. miscelleanous cleanup
@nipunbatra
Copy link
Contributor Author

@fehiepsi I'd already made the Deep kernel learning minimal example and the kernel combinations minimal example, so I let them be for this commit.

I thought it may be good if you can quickly skim the new changes. If they seem to make the tutorial heavy, I think there could be two options

  1. to break down the tutorial into several small pages (like tinyGP, GPyTorch, GPFlow, and even the basic Pyro documentation.
  2. remove some components from this page to keep it concise.

I'd vote for 1 (to break down the tutorial into several small pages), but am happy either way.

@fehiepsi
Copy link
Member

fehiepsi commented Mar 7, 2022

How about either +making blog posts for Pyro GP and adding links to them or +adding those examples to doc strings of the GP modules (e.g. docstring to Binary likelihood, Poisson likelihood, Warping kernel)?

For the current content, it seems to me that:

  • DKL is unnecessary for an introduction tutorial
  • The binary classification example can be removed in favor of the new one for combined kernel

I think we wanted to limit the number of tutorials for the GP module, to be in par with other Pyro modules. At some point in the past, we discussed on splitting the GP module into a separate PyroGP package but didn't have much bandwidth for that (we are a small team so it is necessary to limit our scope). Personally, I was interested in having a Pyro GP framework that uses Pyro primitives and is agnostic to the backend (PyTorch or JAX) but didn't find enough motivation to pursue that. Maybe it is a good item to invest on for the Pyro ecosystem.

@nipunbatra
Copy link
Contributor Author

Hi @fehiepsi Thanks, I have now cleaned up the tutorial, and moved the suggested examples to a separate blog/Jupyter book. Let me know if this looks good to you and is mergeable.

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

fehiepsi commented on 2022-03-09T02:52:54Z
----------------------------------------------------------------

Line #6.    xx, yy = torch.meshgrid(xs, ys)

I think we need to move this into with warnings.catch_warnings(): block with warnings.filter_warnings("ignore") to cleanup the warning output. Alternatively, I guess we can

try:
xx, yy = torch.meshgrid(xs, ys, indexing="xy")

except ...:
xx, yy = torch.meshgrid(xs, ys)

or xx, yy = torch.meshgrid(xs, ys, **mesh_kwargs) where mesh_kwargs = {"indexing": "xy"} for newer torch versions.



@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

fehiepsi commented on 2022-03-09T02:52:55Z
----------------------------------------------------------------

Line #3.    plt.scatter(X, y)

nit: adding plt.show() at the end will remove the matplotlib text in the output


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

fehiepsi commented on 2022-03-09T02:52:56Z
----------------------------------------------------------------

nit: missing plt.show() at the end


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

fehiepsi commented on 2022-03-09T02:52:56Z
----------------------------------------------------------------

Line #5.    plt.plot(X, mean, color='C3', lw=2)

nit: missing plt.show().


@fehiepsi
Copy link
Member

fehiepsi commented Mar 9, 2022

This additional content is awesome! I learned a lot while reading this. Thanks, @nipunbatra! Just have a few small comments above, otherwise I think this is good to merge.

1. cleaned up warning using try, excepy block
2. supressed the matplotlib stray text being printed using _ = ...()
@nipunbatra
Copy link
Contributor Author

Thanks a lot @fehiepsi. I have also learnt a lot :)

I have made all the suggested changes.

@fehiepsi fehiepsi merged commit cef4759 into pyro-ppl:dev Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants