Skip to content

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

Merged
merged 16 commits into from
Apr 8, 2021

Conversation

kolibril13
Copy link
Member

@kolibril13 kolibril13 commented Feb 14, 2021

As there are some example .py scenes, I think a notebook example is a good addition.
image

Depends on #1013

@kolibril13 kolibril13 added the pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! label Feb 14, 2021
Copy link
Member

@behackl behackl left a 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.

@kolibril13
Copy link
Member Author

yep, good point, I will remove it.

@kolibril13
Copy link
Member Author

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.

@naveen521kk
Copy link
Member

Can this be merged @behackl ?

@jsonvillanueva
Copy link
Member

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.

@behackl behackl dismissed their stale review March 21, 2021 10:10

Output has been removed.

@kolibril13
Copy link
Member Author

Here is how the binder link looks now in the docs: https://manimce--1029.org.readthedocs.build/en/1029/installation.html

Copy link
Member

@behackl behackl left a 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.

kolibril13 and others added 3 commits March 23, 2021 08:15
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>
@kolibril13
Copy link
Member Author

Depends on #1013.
I just tried it, and without #1013 this is not working currently:
image

@github-actions github-actions bot added the pr:dependent This PR or issue requires that another PR is merged first label Mar 24, 2021
@jsonvillanueva
Copy link
Member

I'm generally unsure what the appropriate label for this PR is. Maybe documentation?

@github-actions github-actions bot removed the pr:dependent This PR or issue requires that another PR is merged first label Apr 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2021

🎉 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!

@kolibril13 kolibril13 requested a review from behackl April 2, 2021 16:06
@jsonvillanueva jsonvillanueva added the documentation Improvements or additions to documentation label Apr 2, 2021
Copy link
Member

@behackl behackl left a 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.)

@behackl behackl merged commit 2b6b2cd into master Apr 8, 2021
@behackl behackl deleted the example_notebook branch April 8, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants