Implements `-q[l|m|h|k]` #514

jvdoorn · 2020-10-05T14:21:41Z

List of Changes

Implemented -q/--quality [k,h,m,l] flags
Removed -k/-e/-m/-l flags

Motivation

Implements behaviour as described in #146 except for -qp. This flag would specify default behaviour and seems redundant to me. It could however be easily implemented if that is prefered for this PR.

Testing Status

Old tests have been updated to use new flags when needed and a new test has been written to test the flags.

Further Comments

The new test might be a bit too extensive but it covers every way someone could write the flags (par example -q h or -qh are both tested). The production quality does not have its own section in the config but instead reads from the CLI section (which was its old behaviour).

Acknowledgement

I have read the Contributing Guidelines

…llows for easier testing mostly.

leotrs · 2020-10-05T14:29:57Z

manim/config/config.py

+    quality = _determine_quality(args)
+    section = config_parser[quality if quality != "production" else "CLI"]
+
+    # Loop over low quality for the keys, could be any quality really
+    config = {opt: section.getint(opt) for opt in config_parser['low_quality']}


Very clean code, I like it.

leotrs · 2020-10-05T14:30:34Z

manim/constants.py

+    'high_quality': 'h',
+    'medium_quality': 'm',
+    'low_quality': 'l'
+}


Did you run black on this? In any case, please leave a blank line at the end of the file.

leotrs · 2020-10-05T14:37:36Z

manim/config/config_utils.py

    # The following are mutually exclusive and each overrides
    # FRAME_RATE, PIXEL_HEIGHT, and PIXEL_WIDTH,
    parser.add_argument(
-        "-l",


I'm not sure we should completely remove these flags just yet. Doing so will most likely break every single user's workflow. Yes, it's an easy change, but it still needs to be done with care.

Is there a way of adding a deprecation warning instead?

I don't think it is possible to tell -l apart from --low_quality. If we want to warn for deprecation we could maybe create a new argument with just -l and if someone uses it warn them. Not sure if that will produce clean code though.

Hmm that's a good point. I'm still a bit weary of breaking every user's flow though. Let's see what others think.

leotrs · 2020-10-05T14:40:49Z

The test you added is looking good. However, this seems to have broken other existing tests. Could you please check those?

Finally, please make sure to rebuild the documentation as well. Specifically, this PR should change https://manimce.readthedocs.io/en/latest/tutorials/configuration.html and the doctest found therein.

jvdoorn · 2020-10-05T14:43:33Z

Yeah so, when I run just the new test it works. However, when running all tests it fails. I'm not sure what it is caused by, it seems that when running all tests that the parser expects different arguments. I was hoping one of you could enlighten me on the cause of the error it throws.

I'll update the docs!

leotrs · 2020-10-05T14:47:05Z

cc @ManimCommunity/tests

Also, does anyone in @ManimCommunity/core object to this change? I think we should deprecate the current flags instead of removing them.

naveen521kk · 2020-10-05T15:37:10Z

I think this PR should only be merged only after the release. Also, there should should be depreciation warning like done in TexMobject and not abruptly breaking everything.

… a deprecation warning however.

jvdoorn · 2020-10-05T18:48:00Z

I have added a deprecation warning to the old flags but they are still supported. As far as I'm aware this PR is now complete and ready for review.

eulertour

Thanks for writing tests!

eulertour · 2020-10-06T01:33:59Z

manim/config/config_utils.py

+
+    for quality in old_qualities.keys():
+        if getattr(args, quality):
+            logging.getLogger("manim").warning(


If you move this line

manim/manim/config/config.py

Line 167 in 41f6f05

set_rich_logger(config_parser["logger"], file_writer_config["verbosity"])

to just below this line

manim/manim/config/config.py

Line 153 in 41f6f05

args, config_parser, file_writer_config, successfully_read_files = _run_config()

this log will be formatted with rich, which will look a little nicer. You should also update or remove this comment

manim/manim/config/config.py

Line 166 in 41f6f05

# Set the different loggers

since it will only affect a single logger.

kilacoda-old · 2020-10-06T03:36:36Z

CMIIW, but merging this means something like -pm will become -pqm, right? Or will they have to be separated like -p -qm?

jvdoorn · 2020-10-06T06:18:55Z

They don't have to be separated as long as -qm is the last argument.

See Python documentation.

tests/test_scene_rendering/test_cli_flags.py

… logger as well.

jvdoorn · 2020-10-06T07:12:58Z

Uhm, not sure why the tests fail but it seems unrelated to my changes?

eulertour · 2020-10-06T07:38:41Z

Probably transient errors. I tried re-running them, but if its still an unrelated failure we can merge anyway.

jvdoorn · 2020-10-06T07:42:04Z

Well they fail when installing, and since I changed nothing in the install process I'd say it is not possible that my changes broke the tests.

eulertour

Since the old flags are deprecated rather than removed I don't see why this can't be merged now.

huguesdevimeux

Wait, there is not end to end test for these flags (except for -ql).

Do you think it's the right PR for that ? I know, there are already tests for these flags but not invovling the whole video process.
If you decide to do so, everything is explained in the wiki on github, testing section

i mark it as change requested, just to have time to discuss a little bit

eulertour · 2020-10-06T18:38:50Z

Even if you do decide that you want to add end-to-end tests you shouldn't block this PR for it; that should be discussed separately.

huguesdevimeux · 2020-10-06T19:15:28Z

Even if you do decide that you want to add end-to-end tests you shouldn't block this PR for it; that should be discussed separately.

That's why I was asking.

jvdoorn added 10 commits October 5, 2020 13:30

Created a dictionary with the various qualities.

fdf4b27

Implemented the new -q or --quality flag.

07fc1aa

Removed the old -[l|m|e|k] flags.

ac6973f

Replaced old quality flags in tests with the new ones.

3af4e69

Fixed attribute check for q instead of quality.

fe5d825

Moved qualities dict to constants.py.

d3f99e6

Updated help text for -r flag.

eaf428f

Updated changelog.rst.

7894747

Moved the code that determines the quality to a separate file. This a…

657e820

…llows for easier testing mostly.

Wrote extensive tests for the quality flags.

4c62ae6

leotrs reviewed Oct 5, 2020

View reviewed changes

Ran black on constants.py.

ab33ef2

Updated configuration documentation.

438f5c1

jvdoorn added 4 commits October 5, 2020 19:52

Fixed tests by setting input=False when calling _parse_cli().

8341a4a

Fixed style issues using black.

f2d35b0

Added backwards compatibility for the -l/-m/-e/-k flags, you will see…

ebadb15

… a deprecation warning however.

Replaced the print statement with a call to logger.warning.

bfed549

eulertour reviewed Oct 6, 2020

View reviewed changes

huguesdevimeux reviewed Oct 6, 2020

View reviewed changes

tests/test_scene_rendering/test_cli_flags.py Outdated Show resolved Hide resolved

jvdoorn added 2 commits October 6, 2020 09:03

Moved quality cli arg test to a separate file.

59a6235

Moved some lines around so that the deprecation warning uses the rich…

c9364d5

… logger as well.

eulertour approved these changes Oct 6, 2020

View reviewed changes

kilacoda-old approved these changes Oct 6, 2020

View reviewed changes

huguesdevimeux requested changes Oct 6, 2020

View reviewed changes

huguesdevimeux approved these changes Oct 6, 2020

View reviewed changes

eulertour merged commit fa0a82c into ManimCommunity:master Oct 6, 2020

behackl mentioned this pull request Oct 12, 2020

Show inheritance in Documentation #537

Merged

1 task

Implements -q[l|m|h|k] #514

Implements -q[l|m|h|k] #514

Uh oh!

Conversation

jvdoorn commented Oct 5, 2020

List of Changes

Motivation

Testing Status

Further Comments

Acknowledgement

Uh oh!

leotrs Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

leotrs Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

leotrs Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

jvdoorn Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

leotrs Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

leotrs commented Oct 5, 2020

Uh oh!

jvdoorn commented Oct 5, 2020

Uh oh!

leotrs commented Oct 5, 2020

Uh oh!

naveen521kk commented Oct 5, 2020

Uh oh!

jvdoorn commented Oct 5, 2020

Uh oh!

eulertour left a comment

Choose a reason for hiding this comment

Uh oh!

eulertour Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

kilacoda-old commented Oct 6, 2020

Uh oh!

jvdoorn commented Oct 6, 2020

Uh oh!

Uh oh!

jvdoorn commented Oct 6, 2020

Uh oh!

eulertour commented Oct 6, 2020

Uh oh!

jvdoorn commented Oct 6, 2020

Uh oh!

eulertour left a comment

Choose a reason for hiding this comment

Uh oh!

huguesdevimeux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eulertour commented Oct 6, 2020

Uh oh!

huguesdevimeux commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Implements `-q[l|m|h|k]` #514

Implements `-q[l|m|h|k]` #514

huguesdevimeux left a comment •

edited

Loading