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

[Bug]: ax.invert_xaxis() and ax.invert_yaxis() both flip the X axis #21369

Closed
Mars-or-bust opened this issue Oct 15, 2021 · 16 comments
Closed

[Bug]: ax.invert_xaxis() and ax.invert_yaxis() both flip the X axis #21369

Mars-or-bust opened this issue Oct 15, 2021 · 16 comments
Labels
Good first issue Open a pull request against these issues if there are no active ones! topic: mplot3d
Milestone

Comments

@Mars-or-bust
Copy link

Bug summary

The ax.invertxaxis() and ax.invert_yaxis() function both produce the same output, a scatterplot with a flipped X axis.

Code for reproduction

from mpl_toolkits import mplot3d
import matplotlib.pyplot as plt

ax = plt.axes(projection='3d')
plt.title("Invert Z")
ax.scatter3D(1,1,1)
# ax.invert_xaxis()
ax.invert_yaxis()
# ax.invert_zaxis()

Actual outcome

No inversions:
Screen Shot 2021-10-15 at 1 52 25 PM

X Inversion:
Screen Shot 2021-10-15 at 1 52 48 PM

Y Inversion:
Screen Shot 2021-10-15 at 1 54 55 PM

Z Inversion behaves as expected:
Screen Shot 2021-10-15 at 1 56 10 PM

Expected outcome

I would expect the y axis to be inverted

Operating system

No response

Matplotlib Version

3.2.2

Matplotlib Backend

No response

Python version

No response

Jupyter version

No response

Other libraries

mpl_toolkits.mplot3d

Installation

No response

Conda channel

No response

@QuLogic
Copy link
Member

QuLogic commented Oct 15, 2021

The 3D wrappers in lib/mpl_toolkits/mplot3d/axis3d.py are a bit funny. XAxis, YAxis, ZAxis all derive from Axis, which derives from maxis.XAxis. The directional Axis classes override view/data interval setters/getters, so calling set_xlim, etc. work correctly. But set_inverted is not overridden, and calls the x version. Probably there are other methods that are incorrect like that, but happen to work by fluke, or no-one noticed.

@QuLogic
Copy link
Member

QuLogic commented Oct 15, 2021

Something like the following fixes the binding problem:

diff --git a/lib/mpl_toolkits/mplot3d/axis3d.py b/lib/mpl_toolkits/mplot3d/axis3d.py
index 7d6110c25a..cce815eaa6 100644
--- a/lib/mpl_toolkits/mplot3d/axis3d.py
+++ b/lib/mpl_toolkits/mplot3d/axis3d.py
@@ -32,7 +32,7 @@ def tick_update_position(tick, tickxs, tickys, labelpos):
     tick.gridline.set_data(0, 0)
 
 
-class Axis(maxis.XAxis):
+class Axis(maxis.Axis):
     """An Axis class for the 3D plots."""
     # These points from the unit cube make up the x, y and z-planes
     _PLANES = (
@@ -516,21 +516,21 @@ class Axis(maxis.XAxis):
 # Use classes to look at different data limits
 
 
-class XAxis(Axis):
+class XAxis(Axis, maxis.XAxis):
     get_view_interval, set_view_interval = maxis._make_getset_interval(
         "view", "xy_viewLim", "intervalx")
     get_data_interval, set_data_interval = maxis._make_getset_interval(
         "data", "xy_dataLim", "intervalx")
 
 
-class YAxis(Axis):
+class YAxis(Axis, maxis.YAxis):
     get_view_interval, set_view_interval = maxis._make_getset_interval(
         "view", "xy_viewLim", "intervaly")
     get_data_interval, set_data_interval = maxis._make_getset_interval(
         "data", "xy_dataLim", "intervaly")
 
 
-class ZAxis(Axis):
+class ZAxis(Axis, maxis.XAxis):
     get_view_interval, set_view_interval = maxis._make_getset_interval(
         "view", "zz_viewLim", "intervalx")
     get_data_interval, set_data_interval = maxis._make_getset_interval(

but it breaks the Y ticks, probably because of some assumption elsewhere.

@tacaswell tacaswell added this to the v3.6.0 milestone Oct 15, 2021
@tacaswell tacaswell added the Good first issue Open a pull request against these issues if there are no active ones! label Oct 15, 2021
@tacaswell
Copy link
Member

Labeling this as a good first issue because there is a proposed patch and it does not involve any API design choices, however this is not a good issue for someone new-to-programming as it will require chasing through some multiple inheritance issues ;) .

@DebadityaPal
Copy link

DebadityaPal commented Oct 19, 2021

@QuLogic Could you please tell me how it breaks the y-ticks? I tried out the patch but couldn't figure it out.
Also, I would like to work on this issue further and help solve it.

@QuLogic
Copy link
Member

QuLogic commented Oct 19, 2021

The padding is wrong, and sometimes extra grid lines are drawn that shouldn't be. Run the tests and you will see what breaks.

@janash
Copy link

janash commented Oct 20, 2021

I examined this and think that it is actually the tick position which is appearing to throw the padding off. For x-axis, the tick position can be either top or bottom. For the y-axis, it can be left or right. By default, it is set to left (as this is how most 2D plots are oriented).

If you adopt the fix suggested by @QuLogic (fixing inheritance in 3D axes) and add ax.yaxis.set_ticks_position("right") to the plot commands, the spacing is fixed and the inversion is correct.

from mpl_toolkits import mplot3d
import matplotlib.pyplot as plt

ax = plt.axes(projection='3d')
plt.title("Invert Z")
ax.scatter3D(1,1,1)
# ax.invert_xaxis()
ax.invert_yaxis()
ax.yaxis.set_ticks_position("right")

A solution to this may be to create 3D yaxes with different tick and label positions. However, the needed position for the axis will likely depend on the viewing angle of the 3D plot. It does perhaps make sense though, to set the default positions for the 3D y axis to work well with the default viewing angle.

I'd like to submit a pull request to fix this bug, but I may need some additional help on how best to set the position of the ticks for the y axis. Currently, the y-axis for 3D plots looks like this:

class YAxis(Axis, maxis.YAxis):

    get_view_interval, set_view_interval = maxis._make_getset_interval(
        "view", "xy_viewLim", "intervaly")
    get_data_interval, set_data_interval = maxis._make_getset_interval(
        "data", "xy_dataLim", "intervaly")

Would an advisable approach be to override the __init__ method of this class to set the tick positions? I have tried this, but it doesn't successfully change the tick positions.

This is what I have tried:

class YAxis(Axis, maxis.YAxis):

    get_view_interval, set_view_interval = maxis._make_getset_interval(
        "view", "xy_viewLim", "intervaly")
    get_data_interval, set_data_interval = maxis._make_getset_interval(
        "data", "xy_dataLim", "intervaly")

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.offset_text_position = "right"
        self.label_position = "right"
        self.set_ticks_position("right")

and after creating the plots, using the command ax.yaxis.get_ticks_position() still results in left being returned.

Image attached showing that using ax.yaxis.set_ticks_position("right") fixes padding
ticks

@tacaswell
Copy link
Member

tacaswell commented Oct 21, 2021

There are a couple of possible gotcha's in the mplot3D code base. The most 😱 of which is that we do some type promotion where we init the 2D version of the object and then reset the obj.__class__ to make it the 3D version. Another possibility is that in the Axes3D.__init__ we may be re-setting the tick location explicitly someplace else. A third possible thing that could be going wrong is that not all of the classes in the MRO are using super() in their __init__ so the call chain is being broken at some point (either intentionally or accidentally).

My next debugging step here would be to override set_ticks_position like

def set_ticks_position(self, *args, **kwargs):
    breakpoint()
    return super().set_ticks_position(*args, **kwargs)

to sort out where it is being called and to add either breakpoints or prints to the new __init__ to make sure it is really being called.

janash added a commit to janash/matplotlib that referenced this issue Oct 21, 2021
This commit corrects the axis which the 3D y axis inherits from. The y-axis on 3D plots initially inherited from the x axis object, rather than a y axis object. This resulted in a bug where the x axis (instead of the y) was inverted for 3D plots when  was called. After inheritance correction, correct behavior was achieved with inversion, but the tick marks were incorrectly placed. The default location for tick marks on the y-axis is now
set to be right, rather than left.

Resolves matplotlib#21369
@janash janash mentioned this issue Oct 21, 2021
12 tasks
@janash
Copy link

janash commented Oct 21, 2021

Thanks for the tip @tacaswell - it was going through the constructor, but a lot more happens after that. I added the command at a point after the axes are created, may be a bandaid fix. PR opened, lots of tests failing at the moment.

@the-kevin-jr
Copy link

Is this issue still open? I've been looking for a starting point for contribution to open source

@coding-krtek
Copy link

coding-krtek commented Dec 4, 2021

Hello !

So, here's what I've found about the problem that prevents you
from fixing the ticks position.

I tried fixing it by making an init method in the "YAxis"
class in "mpl_toolkits\mplot3d\axis3d.py". The problem is :
this init method will be called before the _get_tick method in
the "YAxis" class in "matplotlib\axis.py". So all the changes
I make are being rewritten, and nothing changes in the end.

The only way I found to bypass this problem is by doing this :

-adding this method in "matplotlib\axis.py" -> YTick class :

...
def fix_init(self):
    self.label1.set(verticalalignment='top',horizontalalignment='center')
    self.label2.set(verticalalignment='top',horizontalalignment='center')

-adding this line in "matplotlib\axis.py" -> YAxis class in the init :

...
def __init__(self, *args, **kwargs):
    self.is3d=kwargs.pop('is3d',None) # added line

-rewriting the _get_tick method in "matplotlib\axis.py" -> YAxis class :

...
def _get_tick(self, major):
    if major:
        tick_kw = self._major_tick_kw
    else:
        tick_kw = self._minor_tick_kw
    my_ytick=YTick(self.axes, 0, major=major, **tick_kw)
    if self.is3d: my_ytick.fix_init()
    return my_ytick

-adding these lines in "mpl_toolkits\mplot3d\axis3d.py" -> YAxis class :

...
get_data_interval, set_data_interval = maxis._make_getset_interval(
    "data", "xy_dataLim", "intervaly")
def __init__(self,*args,**kwargs):              # added line
    super().__init__(*args,is3d=True,**kwargs)  # added line

This is in fact just checking if the projection is 2D or 3D, in order to
change the tick alignments if needed.
and everything works just fine. (including invert.yaxis())
At least I think : I don't know how to check the safety of these addings
properly.

@janash
Copy link

janash commented Dec 5, 2021

@coding-krtek - You can check your solution by running the unit tests. If you have pytest (pip install pytest), you can run the tests by typing pytest into command line when you are in your cloned repo. There are a number of tests which have reference images, and if your solution results in a difference from the stored reference images, the tests will fail. After running the tests, images from the tests will be in a folder called result_images

In my PR for this issue #21424, I fixed the axis inheritance and have changed the behavior in the constructor of Axes3D to change the default side the y axis label is on. This solution is not yet accepted as changing axis inheritance causes changes in the reference images. For your solution did you adopt the proposed patch of fixing the classes which the 3D axes inherit from?

I can't speak for the repo maintainers, but my thought would be that you do not want to change parts of the code (for example YAxis and YTick classes) which don't have anything to do with this issue unless there is no way to avoid it. To rephrase - I think it would be better to keep changes isolated to 3D axis code (closer to the problem, maybe won't be as confusing to someone looking at the code later).

@coding-krtek
Copy link

coding-krtek commented Dec 5, 2021

Um I'll be honest, I was just changing the matplotlib files directly, and I don't know how cloned repositories or virtual environments work.

Yes, I changed the inheritance ...(Axis, maxis.YAxis).
But it looks that the default 2D classes have the last word on the ticks parameters, so I don't know how to solve the problem without changing them.

Edit
Okay so I was wrong, there is a way of fixing the problem without touching the 2D classes :

  • in "mpl_toolkits\mplot3d\axis3d.py" -> YAxis class :
...
class YAxis(Axis, maxis.YAxis):
    get_view_interval, set_view_interval = maxis._make_getset_interval(
        "view", "xy_viewLim", "intervaly")
    get_data_interval, set_data_interval = maxis._make_getset_interval(
        "data", "xy_dataLim", "intervaly")

    #------added lines---------------

    def fix(self):
        self.majorTicks[0].label1.set(horizontalalignment='center',
        verticalalignment='top')
        self.minorTicks[0].label1.set(horizontalalignment='center',
        verticalalignment='top')
        self.majorTicks[0].label2.set(horizontalalignment='center',
        verticalalignment='top')
        self.minorTicks[0].label2.set(horizontalalignment='center',
        verticalalignment='top')

    # If you try to change the position in an __init__ method, it 
    # will be called too soon and the changes will be lost.

    #--------------------------------
  • and then in "mpl_toolkits\mplot3d\axes3d.py" -> Axes3D class, in the "cla" method :
    ...
    def cla(self):
        # docstring inherited.
        super().cla()
        self.zaxis.clear()
        self.yaxis.fix()  #  added line

And this time "everything" (except 11 tests) works, both inheritance and ticks. It's really a duct-tape solution but I don't think there's a way of fixing this issue by changing only the YAxis class and nothing else.

@janash
Copy link

janash commented Dec 6, 2021

Hey @coding-krtek - if you'd like to contribute your solution to matplotlib you'll need to fork the repository and make a pull request. This way, others can see and try out your changes and they might be accepted into the main matplotlib library. You can join the gitter and ask to be put in the "incubator" channel to get help with this - https://gitter.im/matplotlib/matplotlib

@baloe
Copy link

baloe commented Dec 7, 2021

I also noticed that get_ticks_position() returns bottom for all three axes.

@baloe
Copy link

baloe commented Dec 14, 2021

For those in need for a workaround: I discovered that the following lines actually do invert the yaxis:

ylim = ax.get_ylim()
ax.set_yticks( ax.get_yticks() )
ax.set_ylim(ylim[::-1])

@anntzer
Copy link
Contributor

anntzer commented May 27, 2022

Closed by #21442.

@anntzer anntzer closed this as completed May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Open a pull request against these issues if there are no active ones! topic: mplot3d
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants