-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Added example jupyter notebook into the examples folders #1029
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with adding an example notebook, but I'd strongly suggest not including cell output when doing so.
yep, good point, I will remove it. |
As it is convenient to store jupyter notebooks in the manim project folder, I added the jupyter/ folder to the gitignore which ensures that the notebooks are not version controlled. |
Can this be merged @behackl ? |
With #1013, the syntax for jupyter notebooks would change and the sample provided would end up failing. I'm not sure how to test that the notebooks runs without error, but a test case that can be run with pytest would be helpful to ensure the notebook runs properly. |
Here is how the binder link looks now in the docs: https://manimce--1029.org.readthedocs.build/en/1029/installation.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more suggestions, and one further recommendation: I think it would be good if the example notebook also made use of the newly introduced config.media_width
option. A new cell containing something like
# set the maximum width for video outputs to a predefined value
config.media_width = "42%"
should be enough. Then this is (finally) ready to merge, from my point of view.
Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>
Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>
Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>
I'm generally unsure what the appropriate label for this PR is. Maybe documentation? |
🎉 Great news! Looks like all the dependencies have been resolved: 💡 To add or remove a dependency please update this issue/PR description. Brought to you by Dependent Issues (:robot: ). Happy coding! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved the merge conflict and tested locally: LGTM.
(The banner animation seems to be currently broken though, which is unfortunate.)
As there are some example .py scenes, I think a notebook example is a good addition.

Depends on #1013