Skip to content

Conversation

@leotrs
Copy link
Contributor

@leotrs leotrs commented Aug 25, 2020

Some progress in documentation. The changes in this PR can be summarized in three types:

  1. The API reference pages are autogenerated, and the tables looked a bit tight/ugly. So this PR adds 10px padding on them by using a custom css file. This accounts for the diff in custom.css and conf.py.

  2. The autogenerated rst files themselves were littered with blank lines and were hard to read (at least for me). This PR changes the template minimally in order to fix that. This accounts for the diff in class.rst.

  3. I wrote docstrings for Mobject, Group, Mobject.add and Mobject.remove. This accounts for the diff in mobject.py.

  4. I added module-level docstrings to start populating the API reference page. This accounts for the diff in all remaining files.

EDIT: apologies for the messy commit history!

leotrs and others added 30 commits August 15, 2020 14:43
…lines, which is just a symlink to the main repository\'s contributing.md file.
Co-authored-by: Aathish Sivasubrahmanian <[email protected]>
Co-authored-by: Aathish Sivasubrahmanian <[email protected]>
Co-authored-by: Aathish Sivasubrahmanian <[email protected]>
Co-authored-by: Aathish Sivasubrahmanian <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
@kolibril13
Copy link
Member

Do we have yet conventions for names of Mobjects that are

  • a) constructed but not added to a scene
  • b) constructed and added to a scene?

Because at the moment when
talking about b)
we have : "Fading in and out of view" , "arrival on screen" , "The mobjects are added to self.submobjects"
when talking about a)
"Mathematical Object: base class for all objects shown on screen."

Maybe it would be nice to have here something like "constructed Mobjects" and " displayed Mobjects"

@leotrs
Copy link
Contributor Author

leotrs commented Aug 26, 2020

Do we have yet conventions for names of Mobjects that are

  • a) constructed but not added to a scene
  • b) constructed and added to a scene?

Because at the moment when
talking about b)
we have : "Fading in and out of view" , "arrival on screen" , "The mobjects are added to self.submobjects"
when talking about a)
"Mathematical Object: base class for all objects shown on screen."

Maybe it would be nice to have here something like "constructed Mobjects" and " displayed Mobjects"

I'm _so_happy you bring this up. We should definitely have conventions so we are clear and consistent across the documentation. I like "displayed".

Relatedly, I'll just share another convention I was using when writing this PR: Use Mobject only when referring to the actual class. When referring to a single instance of Mobject, whether it is on screen or not, I tried using "mobject" or plural "mobjects".

With these two conventions, we could change the sentences you highlighted to

  • For fading.py: "Display a mobject by fading it in, or stop displaying it by fading it out."
  • Change "arrival" to "the first time it is displayed"
  • "The mobjects are added to self.submobjects" can be changed to "The mobjects are added to self.submobjects. Note this does not display them on screen."
  • "Animate mobjects" would remain with "mobjects" not capitalized.

What do you think?

@kolibril13
Copy link
Member

With these two conventions, we could change the sentences you highlighted to

  • For fading.py: "Display a mobject by fading it in, or stop displaying it by fading it out."
  • Change "arrival" to "the first time it is displayed"
  • "The mobjects are added to self.submobjects" can be changed to "The mobjects are added to self.submobjects. Note this does not display them on screen."
  • "Animate mobjects" would remain with "mobjects" not capitalized.

What do you think?

I like this!
maybe we can also an additional word for "The mobjects are added to self.submobjects", that makes clear that they are now part of the scene, but not displayed.

And do we want to make a new document for naming conventions?
Maybe we can put it in the same file as the changelog.

@leotrs
Copy link
Contributor Author

leotrs commented Aug 26, 2020

maybe we can also an additional word for "The mobjects are added to self.submobjects", that makes clear that they are now part of the scene, but not displayed.

I'm not sure I understood you correctly, so I wanted to check. Here's a scene:

import manim
class TestScene(manim.Scene):
    def construct(self):
        c = manim.Circle()
        s = manim.Square()
        self.add(c)
        c.add(s)
        self.wait(1)

If you run this, it shows both the circle and the square. So the documentation should say something like

Add mobjects as submobjects.

The mobjects are added to self.submobjects.  If the parent mobject is 
displayed in a Scene, the newly-added submobjects will also be displayed 
(i.e. they are automatically added to the parent Scene).

And do we want to make a new document for naming conventions?
Maybe we can put it in the same file as the changelog.

I think maybe the wiki is a better place for this. What do you think?

@kolibril13
Copy link
Member

Add mobjects as submobjects.

The mobjects are added to self.submobjects. If the parent mobject is
displayed in a Scene, the newly-added submobjects will also be displayed
(i.e. they are automatically added to the parent Scene).

Yes, but now that I read it, I think it is better to keep it simple, without the extra sentence that might cause more confusion then benefit.

To make this sentence clear, maybe we can add your example later to the documentation examples

@leotrs
Copy link
Contributor Author

leotrs commented Aug 27, 2020

Yes, but now that I read it, I think it is better to keep it simple, without the extra sentence that might cause more confusion then benefit.

That's fair. I'll add that sentence in the notes section instead.

@leotrs
Copy link
Contributor Author

leotrs commented Aug 27, 2020

@kolibril13 I just pushed a bunch of changes after your review. Let me know if you think this is ready to be merged!

@huguesdevimeux huguesdevimeux added the waiting for requested review PR is blocked by a missing requested review label Aug 27, 2020
Copy link
Member

@huguesdevimeux huguesdevimeux left a comment

Choose a reason for hiding this comment

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

SGTM !

@huguesdevimeux huguesdevimeux added pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! and removed waiting for requested review PR is blocked by a missing requested review labels Aug 28, 2020
@leotrs
Copy link
Contributor Author

leotrs commented Aug 28, 2020

Most comments have been addressed, just waiting on confirmation by @kolibril13 :)

@kolibril13
Copy link
Member

Approved!

@leotrs leotrs merged commit 662777b into master Aug 28, 2020
@leotrs leotrs deleted the docs branch August 28, 2020 13:20
Copy link
Member

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

pepe

Parameters
----------
mobjects : List[:class:`Mobject`]
Copy link
Member

Choose a reason for hiding this comment

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

we aren't expecting lists of mobjects, right?

Suggested change
mobjects : List[:class:`Mobject`]
mobjects : :class:`Mobject`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mobjects is always a list, no? How to specify it is packing all arguments passed?

See Also
--------
:meth:`~Mobject.remove`
Copy link
Member

Choose a reason for hiding this comment

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

i mean this can be shortened to just "remove" but we can let it be i guess

Parameters
----------
mobjects : List[:class:`Mobject`]
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
mobjects : List[:class:`Mobject`]
mobjects : :class:`Mobject`

@PgBiel PgBiel mentioned this pull request Sep 2, 2020
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.

6 participants