Deprecate internal functions exposed in the public API of mplot3d by rth · Pull Request #13030 · matplotlib/matplotlib

rth · 2018-12-20T15:40:21Z

mplot3d currently exposes most (if not all) internal functions as part of the public API, making changes very difficult. This deprecates some of these internal functions, which is a pre-requisite for #11577

Following functions were deprecated,

- art3d.norm_angle
- art3d.norm_text_angle
- art3d.path_to_3d_segment
- art3d.paths_to_3d_segments
- art3d.path_to_3d_segment_with_codes
- art3d.paths_to_3d_segments_with_codes
- art3d.get_colors
- art3d.zalpha
- proj3d.line2d (never used)
- proj3d.line2d_dist (never used)
- proj3d.mod
- proj3d.proj_transform_vec
- proj3d.proj_transform_vec_clip
- proj3d.vec_pad_ones
- proj3d.proj_trans_clip_points (never used)

In #11577, the following functions would also change of output type,

proj3d.proj_transform
proj3d.proj_transform_clip
proj3d.transform
proj3d.proj_trans_points

I'm not sure if I should deprecate these too, of it they should remain as part of the public API in the long term.

Note: also checked that no deprecated functions are used with,

pytest -Werror lib/mpl_toolkits/tests/test_mplot3d.py

QuLogic · 2018-12-23T03:09:31Z

doc/api/api_changes/2018-12-20-AL-deprecations.rst

+````````````
+
+Multiple internal functions that were exposed as part of the public API
+of ``mpl_toolkits.mplot3d`` are deprecated.


Please list them all.

timhoffm

Looks, good. Approved conditionally; as @QuLogic mentioned, all deprecated functions must be listed explicitly in the deprecation note.

tacaswell

I am only requesting changes to make sure @WeatherGod has a chance to review this.

@WeatherGod should dismiss this review.

WeatherGod · 2019-01-07T16:50:59Z

lib/mpl_toolkits/mplot3d/proj3d.py

+    return _proj_transform_vec(vec, M)
+
+
+def _proj_transform_vec_clip(vec, M):


These deprecations might be a bit problematic. IIRC, some of these functions in proj3d.py are used elsewhere in 3rd party tools like mplcursors.

particularly, the thing I am thinking of is any code that tries to infer the 3d coordinates from a 2D location, so that coordinates can be displayed in the interactive text display in the lower right-hand corner of the figure window.

yeah, line2d_seg_dist() is what is used.

WeatherGod · 2019-01-07T17:04:55Z

This PR's summary should have the list of deprecated functions updated, and the list needs to be included in the api changes notes.

While I am fairly certain that most of these functions were never used, some were used. Deprecating these without providing a path forward for users is a bit jarring to me.

anntzer · 2019-01-07T18:10:09Z

mplcursors is definitely not using any of these.
It has its own _compute_projection_pick for similar stuff, but frankly even if I didn't want to implement the function myself mplot3d would not have been the place I'd have looked for an implementation.

WeatherGod · 2019-01-07T18:21:59Z

Sorry, I meant mpldatacursor: https://github.com/joferkington/mpldatacursor/blob/7dabc589ed02c35ac5d89de5931f91e0323aa795/mpldatacursor/pick_info.py#L296

rth · 2019-01-07T18:23:03Z

Thanks for the reviews! Will update the docs for API changes.

While I am fairly certain that most of these functions were never used, some were used. Deprecating these without providing a path forward for users is a bit jarring to me.

Well, the alternative is to keep them public, but then it would be difficult to change anything in mplot3d as currently, everything is public and different functions are tightly coupled.

For instance, regarding proj_transform_vec_clip on which you commented. It is not documented, and so one could argue that it is not part of the public API by the PEP8 definition https://www.python.org/dev/peps/pep-0008/#public-and-internal-interfaces

particularly, the thing I am thinking of is any code that tries to infer the 3d coordinates from a 2D location, so that coordinates can be displayed in the interactive text display in the lower right-hand corner of the figure window.

Which of the functions deprecated here should be kept public then?

anntzer · 2019-01-07T18:24:47Z

If @WeatherGod doesn't want any of these to be deprecated, I'd suggest walling off these functions to their own corner, duplicate them to private (underscore-prefixed) versions, modify the private versions accordingly, and make mplot3d internally use the internal version of all these functions.
I think these backcompat considerations have taken far too big a toll on real performance improvements.

WeatherGod · 2019-01-07T20:18:01Z

I am not against deprecating these functions, I just want to make sure there is a path forward documented, particularly for the few functions that are known to be used in-the-wild.

anntzer · 2019-01-07T20:19:39Z

The path forward documented is to copy-paste the old implementation...

WeatherGod · 2019-01-07T20:30:29Z

Where is that documented? And, for some of these deprecated functions, doing so would be wrong because they will call other internal functions.

anntzer · 2019-01-07T20:32:34Z

"... that will be documented..."
(and yes you'll have to look up for dependencies and copy them too)

addressed.

tacaswell · 2019-01-07T21:08:50Z

I am 👍 on this modulo listing the function in the deprecation warning and adding a sentence that if you rely on these functions to vendor them.

WeatherGod · 2019-01-07T21:31:39Z

In addition, this PR is still incomplete because not all uses of the deprecated functions have been found and handled. Relying on pytest to find these for you is misleading because matplotlib's test coverage isn't the best. The one that I noticed off the bat is the use of line2d_seg_dist() in axes3d.py that was missed. I don't know if there are others.

WeatherGod

Need to find the remaining uses of the deprecated functions.

NelleV · 2019-01-18T03:05:27Z

@rth Do you think you could finish up the work that needs to be done on this PR? It should not be a lot of work.

rth · 2019-01-22T10:25:44Z

Thanks for all the feedback.

not all uses of the deprecated functions have been found and handled.

Re-run grep for all deprecated functions, and found a few that were indeed missing.

listing the functions in the deprecation warning and adding a sentence that if you rely on these functions to vendor them.

Added the list of all deprecated functions to the release notes and added a sentence suggesting vendoring.

Please let me know if I need to do anything else.

jklymak · 2019-01-28T20:52:47Z

ping @WeatherGod We are hoping to tag 3.1 soonish and this should be in there...

WeatherGod · 2019-02-06T01:43:40Z

lib/mpl_toolkits/mplot3d/axes3d.py

        self.eye = E
        self.vvec = R - E
-        self.vvec = self.vvec / proj3d.mod(self.vvec)
+        self.vvec = self.vvec / proj3d._mod(self.vvec)


This is going to conflict with #13020.

I'll rebase #13020 on top of this after this is merged.

WeatherGod · 2019-02-06T01:44:17Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        shade = np.array([np.dot(n / proj3d.mod(n), lightsource.direction)
-                          if proj3d.mod(n) else np.nan
+        shade = np.array([np.dot(n / proj3d._mod(n), lightsource.direction)
+                          if proj3d._mod(n) else np.nan


Going to conflict with #13020

WeatherGod · 2019-02-06T01:48:21Z

lib/mpl_toolkits/mplot3d/proj3d.py

+@cbook.deprecated("3.1")
+def mod(v):
+    """3d vector length"""
+    return _mod(v)


All of these mod related things should be superceded by #13020, because it is straight-forward to just use np.linalg.norm().

efiring · 2019-02-12T03:07:36Z

@WeatherGod, if I understand correctly, all of your present reservations relate to the interaction with #13020, which @anntzer will rebase as needed. Is there anything that still needs to be fixed in this PR, apart from a rebase?

WeatherGod · 2019-02-12T14:53:18Z

Yes, sorry if I wasn't clear. Is the Travis failure unrelated to the PR?

jklymak · 2019-02-12T15:00:52Z

flake8 needs to pass!

rth · 2019-02-12T16:35:17Z

Sorry, merging master introduced the flake8 issue. Should be fixed now.

anntzer · 2019-02-12T17:05:34Z

thanks!

rth and others added 3 commits December 20, 2018 16:26

Deprecate functions in art3d.py

ae83f2c

More deprecations in proj3d

8147734

Add API change entry

e4a983f

rth mentioned this pull request Dec 20, 2018

[MRG] Optimize 3D display (continued) #11577

Closed

10 tasks

QuLogic requested a review from WeatherGod December 23, 2018 03:09

QuLogic approved these changes Dec 23, 2018

View reviewed changes

QuLogic mentioned this pull request Dec 23, 2018

Deprecate proj3d.mod. #13020

Merged

6 tasks

timhoffm approved these changes Dec 23, 2018

View reviewed changes

tacaswell previously requested changes Dec 24, 2018

View reviewed changes

dstansby added the topic: mplot3d label Dec 26, 2018

WeatherGod reviewed Jan 7, 2019

View reviewed changes

tacaswell added this to the v3.1 milestone Jan 7, 2019

WeatherGod requested changes Jan 7, 2019

View reviewed changes

anntzer mentioned this pull request Jan 20, 2019

Vectorize mplot3d.art3d.zalpha. #13235

Merged

6 tasks

rth added 3 commits January 22, 2019 10:18

List all deprecated fuctions

4bc0e4a

Fix missing uses of deprecated functions

a55a4ea

Suggest vendoring in the deprecation notice

661deec

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 28, 2019

WeatherGod reviewed Feb 6, 2019

View reviewed changes

Merge branch 'master' into private-funcs-mplot3d

9f11f64

WeatherGod approved these changes Feb 12, 2019

View reviewed changes

Lint

f406cc1

anntzer merged commit b6e8c19 into matplotlib:master Feb 12, 2019

		return _proj_transform_vec(vec, M)


		def _proj_transform_vec_clip(vec, M):

Uh oh!

Conversation

rth commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuLogic Dec 23, 2018

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

WeatherGod Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

WeatherGod Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

WeatherGod Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

WeatherGod commented Jan 7, 2019

Uh oh!

anntzer commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WeatherGod commented Jan 7, 2019

Uh oh!

rth commented Jan 7, 2019

Uh oh!

anntzer commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WeatherGod commented Jan 7, 2019

Uh oh!

anntzer commented Jan 7, 2019

Uh oh!

WeatherGod commented Jan 7, 2019

Uh oh!

anntzer commented Jan 7, 2019

Uh oh!

tacaswell commented Jan 7, 2019

Uh oh!

WeatherGod commented Jan 7, 2019

Uh oh!

WeatherGod left a comment

Choose a reason for hiding this comment

Uh oh!

NelleV commented Jan 18, 2019

Uh oh!

rth commented Jan 22, 2019

Uh oh!

jklymak commented Jan 28, 2019

Uh oh!

WeatherGod Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

anntzer Feb 6, 2019 • 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!

WeatherGod Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

WeatherGod Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

efiring commented Feb 12, 2019

Uh oh!

WeatherGod commented Feb 12, 2019

Uh oh!

jklymak commented Feb 12, 2019

Uh oh!

rth commented Feb 12, 2019

Uh oh!

anntzer commented Feb 12, 2019

Uh oh!

Reviewers

rth commented Dec 20, 2018 •

edited

Loading

anntzer commented Jan 7, 2019 •

edited

Loading

anntzer commented Jan 7, 2019 •

edited

Loading

anntzer Feb 6, 2019 •

edited

Loading