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

Fix: NumberLine number_to_point broken when using add_tip #3820

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

Conversation

goldenphoenix713
Copy link
Contributor

@goldenphoenix713 goldenphoenix713 commented Jun 22, 2024

Overview: What does this pull request change?

Fixed issue where NumberLine's number_to_point method didn't convert from number line value to image position correctly after calling add_tip after creation. See issue #3740.

Motivation and Explanation: Why and how do your changes improve the library?

NumberLine's number_to_point method now converts from number line value to image position correctly after calling add_tip.

Links to added or changed documentation pages

No documentation changed.

Further Information and Comments

Also fixed was a small issue with the final tick mark of a number line not always appearing when a tip was added.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

… end positions of an object would include the length of an added tip, instead of the expected length without the tip.
…ot appear. Also, the size of a tip when added after creation with default parameters did not match the size when added during creation.
@goldenphoenix713
Copy link
Contributor Author

There's an issue now where the expected values in several of the tests (the ones that depend on NumberLine or classes that implement NumberLine, e.g., Axes don't match the new values computed whenever there's a n2p or p2n method called. Since the calculated values are, in fact, different due to the bug fix, the expected values in the tests will need to be changed to reflect the new calculated values. Please advise on how to proceed.

@goldenphoenix713 goldenphoenix713 marked this pull request as ready for review June 22, 2024 22:23
@JasonGrace2282
Copy link
Member

Hello! Thanks for taking on this issue, and helping out with developing Manim :)

Since the calculated values are, in fact, different due to the bug fix, the expected values in the tests will need to be changed to reflect the new calculated values. Please advise on how to proceed.

You can see how to regenerate the graphical data in our docs

However, I would think carefully before you do, and look at the difference between your results and the current test data. E.g. for the test tests/test_graphical_units/test_coordinate_systems.py::test_plot_surface, using pytest --show_diff tests/test_graphical_units/test_coordinate_systems.py::test_plot_surface gives me this

Some possible options to explore is a boolean parameter to toggle between get_start/base, or if you think of anything better feel free to try that out too :)

@goldenphoenix713
Copy link
Contributor Author

goldenphoenix713 commented Jun 23, 2024

I think one possible way forward is to have an input parameter that control whether the arrow tip isn't create insides or outside of the end of the line. The default value of this parameter can be the original implementation (with the tip generated inside the end of the line), but it would also allow users to configure the arrow tips to their liking. By setting the default to the tip inside, the tests (hopefully) should generate data that matches the expected values.

My guess is we'd have to add this new parameter to all classes that use / are subclasses of TipableVMobject

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

2 participants