Skip to content

Conversation

@Zarty55
Copy link
Contributor

@Zarty55 Zarty55 commented Oct 21, 2020

List of Changes

When x_min or y_min were non-zero values the axis would shift and be separated. Now the axis should stay together.

Motivation

When graphing functions that are far off the origin point (0, 0) you normally whant to translate the origin. The original behaviour did not produce a satisfactory output because the axis were separated.

Explanation for Changes

Simply shifting each axis by -axis.number_to_point(min_val)

Testing Status

Checking this minimum working example:

class GraphShift(GraphScene):
	CONFIG = {
	    "x_min": 1,
	    "x_max": 5,
	    "x_axis_width": 8,
	    "x_tick_frequency": 0.25,
	    "x_labeled_nums": [1, 2, 3, 4, 5],

	    "y_min": 0.25,
	    "y_max": 1.25,
	    "y_axis_height": 5,
	    "y_tick_frequency": 0.25,
	    "y_labeled_nums": [0.25, 0.5, 0.75, 1, 1.25]
		}

	def construct(self):
		self.setup_axes()
		sine = self.get_graph(lambda x: 0.25*np.sin((x - 1) * np.pi/2) + 0.5)
		self.play(ShowCreation(sine))

Which produces this graph
Image

Applying the changes we get

Image2
Which is much better. There is actually an error on the ticks of the y axis, it should be [0.25, 0.5, 0.75, 1, 1.25] but it seems to be rounded. I'm not sure if this is an error on my part, though.

Further Comments

This is my first pull request to any VCS ever, so if I made any mistakes please explain to me how to correct them.

When x_min or y_min were non-zero values the axis would shift and be separated. Now the axis should stay together.
@leotrs
Copy link
Contributor

leotrs commented Oct 21, 2020

Hi @Zarty55, thank you for your contribution! This is certainly a welcome fix.

However, your changes broke some of the tests. Pinging @ManimCommunity/tests team to see if they can chime in. Note there is a possibility that your changes are correct and it is actually the tests that are outdated. Not sure at the moment.

@huguesdevimeux
Copy link
Member

The test is failing but I think it's literaaly the test that has been outdated byt this change.
Could you fix the test? You'll find everything you need on the wiki.
I will ask you to share what you get when you run pytest -x --show_diff (there should be a new windows opening with what differ in the tests).

@huguesdevimeux huguesdevimeux added the test requested Implementation of tests are requested label Oct 22, 2020
@huguesdevimeux huguesdevimeux marked this pull request as draft October 22, 2020 06:06
@Zarty55
Copy link
Contributor Author

Zarty55 commented Oct 22, 2020

Alright, I will try to check it probably tomorrow or during the weekend.

I actually just noticed that this will not work anymore to display in the way most people will actually use (with the y axis in the middle, this change always places the y axis on the left). Maybe a better behaviour would be to check if the interval [x_min, x_max] contains 0 and if not then apply the shift to not have the axis be separated.

@kolibril13 kolibril13 mentioned this pull request Oct 22, 2020
@kolibril13
Copy link
Member

#574

Maybe a better behaviour would be to check if the interval [x_min, x_max] contains 0 and if not then apply the shift to not have the axis be separated.

Good idea!

@Zarty55 : The script to test examples was repaired in #574 a few minutes ago, so make sure you pull the latest master before you start testing ;)

@behackl
Copy link
Member

behackl commented Nov 11, 2020

Hello @Zarty55! Due to inactivity on this PR, I take over and finalize your fix.

It turns out that this needed a little rewrite: instead of always shifting the axes to x_min, y_min, this only moves them if either x_min <= 0 <= x_max or y_min <= 0 <= y_max is not satisfied. This was what made our tests fail.

I've also added some (non-graphical) unit tests to check that everything is shifted correctly. Ready to review.

@behackl behackl marked this pull request as ready for review November 11, 2020 01:37
@leotrs
Copy link
Contributor

leotrs commented Nov 11, 2020

Don't forget to update the changelog 😉

@kolibril13
Copy link
Member

image
In this plot, I have now two strange lines.
Any ideas why?

from manim import *

class SinAndCosFunctionPlot(GraphScene):
    CONFIG = {
        "x_min": 1,
        "x_max": 10.3,
        "num_graph_anchor_points": 100,
        "y_min": -1.5,
        "y_max": 1.5,
        "graph_origin": LEFT,
        "function_color": RED,
        "axes_color": GREEN,
        "x_labeled_nums": range(1, 12, 2),
    }

    def construct(self):
        self.setup_axes(animate=False)
        func_graph = self.get_graph(np.cos, self.function_color)
        func_graph2 = self.get_graph(np.sin)
        vert_line = self.get_vertical_line_to_graph(TAU, func_graph, color=YELLOW)
        graph_lab = self.get_graph_label(func_graph, label="\\cos(x)")
        graph_lab2 = self.get_graph_label(func_graph2, label="\\sin(x)",
                                          )
        two_pi = MathTex(r"x = 2 \pi")
        label_coord = self.input_to_graph_point(TAU, func_graph)
        two_pi.next_to(label_coord, RIGHT + UP)
        self.add(func_graph, func_graph2, vert_line, graph_lab, graph_lab2, two_pi)

SinAndCosFunctionPlot

@behackl
Copy link
Member

behackl commented Nov 13, 2020

In this plot, I have now two strange lines.
Any ideas why?

It took me a while to realize it: these strange lines are actually a 1 that is hidden below the y axis. 😄

@behackl
Copy link
Member

behackl commented Nov 13, 2020

Don't forget to update the changelog 😉

Thanks for the reminder! Done via 9d38c7b

@kolibril13 kolibril13 merged commit e4f5070 into ManimCommunity:master Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test requested Implementation of tests are requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants