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

Allow users to define custom marker cells #108

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

Conversation

bhoov
Copy link
Collaborator

@bhoov bhoov commented Dec 31, 2024

Expose the manim-cellMarker (which defaults to ##) as a user-configurable option. A possible solution to #12, since it allows users to define a cell marker that would not clash with their other python files.

Proposal. @Splines, this is not yet implemented but I would like your feedback. In the spirit of minimizing clashing with other python files, I propose to change the default cell marker from ## to #:. The double comment is common in my existing python code, perhaps because I wanted to emphasize the comment, or because I commented a region that contained comments. What do you think?

@bhoov bhoov requested a review from Splines December 31, 2024 16:24
@Splines
Copy link
Member

Splines commented Dec 31, 2024

A possible solution to #12, since it allows users to define a cell marker that would not clash with their other python files.

For a solution to #12, I'd like to rather implement the heuristic I've described here. This would also allow to only show the export scene codelens on actual Manim classes and would be more robust in the long term.

  • Assume that said heuristic were already implemented. Then I don't see the immediate benefit of letting users customize the cell marker. The argument to avoid clashes would then not be applicable anymore since the heuristic would take care of that.
  • An argument for not exposing this as config option would be that in samples we provide and samples that users share on the net, they can always use that ## syntax and others don't have to adapt it when copy-pasting the code. In that sense, it's a bit of a "brand" ;) Manim Cell = ##
  • If we still want to expose this as config, I favor a default of ## over # because maybe I want to also write some comments here and there in my Manim code. However, the default in Manim itself is indeed #, so Manim would detect that as a checkpoint (but that can be safely ignored, it doesn't have any negative consequences and performance for creating a few more checkpoints should really be neglegible).

So then, what would the benefit of a customization of the cell markers be? Probably "just for fun" when users don't like ## for aesthetic reasons? But in this case, maybe waiting for an issue created by an external user and emoji-feedback on that issue would be a better idea?

@Splines
Copy link
Member

Splines commented Dec 31, 2024

Also note that right now ####### is also detected as Manim Cell. We should agree on whether only ## or also an arbitrary number of hashes (bigger than 1) should be detected as Manim cell. I vote for just the ## as Manim Cell. Everything else is not a Manim Cell, e.g. not #, ###, #### etc.

@bhoov
Copy link
Collaborator Author

bhoov commented Jan 6, 2025

Gotcha. I was just trying to simplify the dev work -- I agree that there is no need to expose this config to the user if the manim file detection works well. I do not mind closing this PR if the auto detection works as expected

We should agree on whether only ## or also an arbitrary number of hashes (bigger than 1) should be detected as Manim cell

My default response is to emulate the behavior of the #%% jupyter extension, which allows arbitrary number of pcts to register as a jupyter cell (e.g., #%%%% is a valid cell marker). Do you see any issues with allowing ##### to be a valid manim cell marker?

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.

2 participants