/matplotlib

Orthographic projection for mplot3d #8326

Merged
merged 7 commits into from Apr 10, 2017
+648 −2

Conversation

Reviewers

@thisch thisch

@dstansby dstansby

@WeatherGod WeatherGod

Assignees
No one assigned
Labels
mplot3d
Projects
None yet
Milestone
  2.1 (next point r...
4 participants
@txxia @WeatherGod @dstansby @thisch
@txxia
Contributor

txxia commented Mar 18, 2017 • edited

Fixes #537

  • implemented ortho_transformation() in proj3d
  • added a flag persp and a setter set_persp()

Here is a simple demo:

import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import proj3d
fig = plt.figure()
def init_axes(ax):
    ax.set_aspect("equal")
    ax.set_xlabel('x')
    ax.set_ylabel('y')
    ax.set_zlabel('z')
ax = fig.add_subplot(121, projection='3d', persp=False)
init_axes(ax)
ax = fig.add_subplot(122, projection='3d', persp=True)
init_axes(ax)
plt.show()

figure_1
@txxia txxia implemented orthographic projection

Some checks were not successful

1 failing and 4 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
codecov/patch — 100% of diff hit (target 80%)
Details
codecov/project/library — 57.97% (+<.01%) compared to 7a152d8
Details
codecov/project/tests — 98.55% (target 97.9%)
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
fa7784c

dstansby added the mplot3d label Mar 18, 2017

dstansby added this to the 2.1 (next point release) milestone Mar 18, 2017

@dstansby

dstansby requested changes Mar 19, 2017
View changes

Neat little new feature! Just a couple of implementation changes I've suggested.

lib/mpl_toolkits/mplot3d/axes3d.py
@@ -63,6 +63,7 @@ def __init__(self, fig, rect=None, *args, **kwargs):
*elev* Elevation viewing angle (default 30)
*zscale* [%(scale)s]
*sharez* Other axes to share z-limits with
+ *persp* Perspective projection (default True)
@dstansby

dstansby Mar 19, 2017

Contributor

I think it would be better to have a proj argument instead, which (at the moment) could take either 'persp or ortho values, and default to persp.

@txxia

txxia Mar 19, 2017

Contributor

I agree. In that case, should I provide the setter set_proj? There is a get_proj which has different meaning.

@dstansby

dstansby Mar 19, 2017

Contributor

Maybe use projection instead of proj to avoid breaking get_proj?

I think a better alternative would be to rename the current get_proj to get_proj_matrix - might be worth seeing what other devs think about that.

lib/mpl_toolkits/tests/test_mplot3d.py
+ fig = plt.figure()
+ ax = fig.gca(projection='3d')
+ ax.set_persp(False)
+ ax.set_xlabel('X')
@dstansby

dstansby Mar 19, 2017

Contributor

This test doesn't really need any text, so could the label lines be removed.

@txxia txxia ortho projection fixes 
- renamed persp to proj
- renamed get_proj to get_proj_matrix
- add test for ortho_projection

Some checks were not successful

2 failing and 3 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
codecov/patch — 91.83% of diff hit (target 80%)
Details
codecov/project/library — Absolute coverage decreased by -0.22% but relative coverage increased by +18.5% compared to 7a152d8
Details
codecov/project/tests — 98.56% (target 97.9%)
Details
579d1e4
@txxia
Contributor

txxia commented Mar 20, 2017

I am not too sure about this. The name projection conflicts with the flag in add_subplot (see above demo), but renaming get_proj may break users' code if they happen to be using it.
lib/mpl_toolkits/tests/test_mplot3d.py
# eye point
E = np.array([1000, -1000, 2000])
R = np.array([100, 100, 100])
V = np.array([0, 0, 1])
viewM = proj3d.view_transformation(E, R, V)
- perspM = proj3d.persp_transformation(100, -100)
@txxia

txxia Mar 20, 2017 • edited

Contributor

I am not sure about this either. This seems to assume zfront and zback refer to the coordinates on the camera's Z axis (which is backwards), but judging from other places where it's called, they seem to be equivalent to near / far plane.
https://github.com/matplotlib/matplotlib/blob/master/lib/mpl_toolkits/mplot3d/axes3d.py#L1001

@WeatherGod

WeatherGod Mar 29, 2017

Member

it is entirely possible this test was originally backwards. It came from some commented out code at the bottom of proj3d.py. Seemed like it might have been some debugging code for the original developer. We recently lifted it out of there and put it in as a unit test because it helped us solve a problem.

@txxia

txxia Mar 29, 2017

Contributor

Ah, thanks for the clarification. I will leave it as is then.

@WeatherGod
Member

WeatherGod commented Mar 29, 2017

it is unfortunate name choice, but get_proj() has been a part of the public API for a very long time now. I do know of code in the wild that uses it. Please do not change that.
lib/mpl_toolkits/mplot3d/axes3d.py
- def get_proj(self):
+ def set_proj(self, proj):
+ """
+ Set the type of projection.
@WeatherGod

WeatherGod Mar 29, 2017

Member

Need to expand on this; detail what are the accepted inputs and what they mean.

@txxia

txxia Mar 29, 2017

Contributor

Do you think it will be okay if I rename the flag and this setter to proj_type? since I will keep get_proj, using set_proj would cause confusion.

@WeatherGod

WeatherGod Mar 30, 2017

Member

Yes, I think your new setter/getter can be proj_type. I am a little leery of using the word "type" in a variable name when one is not referring to a type object, but I think it is ok here, especially if it is documented well.

lib/mpl_toolkits/tests/test_mplot3d.py
# eye point
E = np.array([1000, -1000, 2000])
R = np.array([100, 100, 100])
V = np.array([0, 0, 1])
viewM = proj3d.view_transformation(E, R, V)
- perspM = proj3d.persp_transformation(100, -100)
@WeatherGod

WeatherGod Mar 29, 2017

Member

it is entirely possible this test was originally backwards. It came from some commented out code at the bottom of proj3d.py. Seemed like it might have been some debugging code for the original developer. We recently lifted it out of there and put it in as a unit test because it helped us solve a problem.

lib/mpl_toolkits/mplot3d/proj3d.py
+ [0,2,0,0],
+ [0,0,-2,0],
+ [0,0,a,b]
+ ])
@WeatherGod

WeatherGod Mar 29, 2017

Member

Can you provide a reference for this formulation? I am not an expert in 3d transforms, but this doesn't match anything I am seeing on the wikipedia page for orthographic projection.

@txxia

txxia Mar 29, 2017 • edited

Contributor

Sorry, I should have justified it.
This matrix is almost equivalent to the one in the Wikipedia page. Based on persp_transformation, I made these assumptions:

  • zfront is the value for near, left and bottom
  • zback for far, right, top
  • the X and Y translations are omitted (I guess so the image will always be centered at the origin)

It's a simple derivation from the one from Wikipedia (v1), but since v2 and v1 are equivalent in homogeneous coordinates, I thought the second will look nicer.

@WeatherGod

WeatherGod Mar 30, 2017

Member

ah, and if I transpose that, it then matches up. Got it. Well, just include some notes about it in comments for some future poor soul.

txxia added some commits Apr 1, 2017

@txxia txxia Revert changes to the existing test acbad48
@txxia txxia rename flag to proj_type, update tests

Some checks were not successful

1 failing and 4 successful checks
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
codecov/patch — 94.28% of diff hit (target 80%)
Details
codecov/project/library — 58.05% (+0.08%) compared to 7a152d8
Details
codecov/project/tests — 98.58% (target 97.9%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
6025b0d
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -961,9 +961,15 @@ def view_init(self, elev=None, azim=None):
else:
self.azim = azim
- def set_proj(self, proj):
+ def set_proj_type(self, proj):
@thisch

thisch Apr 1, 2017

Contributor

why didn't you rename the argument name as well?

@txxia

txxia Apr 1, 2017

Contributor

Thanks for pointing out, I just renamed the argument.

@txxia txxia rename parameter in the setter

All checks have passed

5 successful checks
codecov/patch — 94.28% of diff hit (target 80%)
Details
codecov/project/library — 58.05% (+0.08%) compared to 7a152d8
Details
codecov/project/tests — 98.58% (target 97.9%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
21a8085
@WeatherGod
Member

WeatherGod commented Apr 3, 2017

This looks great. The only thing left to do is to add an entry to the what's new page: There should be some notes in doc/users/whats_new/

txxia added some commits Apr 4, 2017

@txxia txxia added what's new file

All checks have passed

5 successful checks
codecov/patch — 94.28% of diff hit (target 80%)
Details
codecov/project/library — 58.08% (+0.11%) compared to 7a152d8
Details
codecov/project/tests — 98.58% (target 97.9%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
51d8018
@txxia txxia include 'axes3d' in the file name

All checks have passed

5 successful checks
codecov/patch — 94.28% of diff hit (target 80%)
Details
codecov/project/library — 58.08% (+0.11%) compared to 7a152d8
Details
codecov/project/tests — 98.58% (target 97.9%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
cc425f6
@txxia
Contributor

txxia commented Apr 4, 2017

I am new to numpydoc. The file I created seems to compile, but I had trouble generating everything under doc/ (it gave KeyError: b'tcsi1728' at some point). Should I worry about that?
@WeatherGod
Member

WeatherGod commented Apr 4, 2017

we build the docs as part of our test suite. If it builds there, that should be sufficient.
@WeatherGod
Member

WeatherGod commented Apr 5, 2017

It would be nice to get more documentation for this feature, especially prose and graphics showing when this feature makes a significant improvement. However, that can be done in a separate PR.
@WeatherGod
Member

WeatherGod commented Apr 7, 2017

@dstansby , I take it that your concerns have been addressed?
@dstansby
Contributor

dstansby commented Apr 9, 2017

@WeatherGod WeatherGod merged commit 6f4cc13 into matplotlib:master Apr 10, 2017
5 checks passed

5 checks passed

codecov/patch 94.28% of diff hit (target 80%)
Details
codecov/project/library 58.08% (+0.11%) compared to 7a152d8
Details
codecov/project/tests 98.58% (target 97.9%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@WeatherGod
Member

WeatherGod commented Apr 10, 2017

Thanks to everybody!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment