NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Deprecate internal functions exposed in the public API of mplot3d#13030

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

anntzer merged 8 commits intomatplotlib:masterfromrth:private-funcs-mplot3d

Feb 12, 2019

Merged

Deprecate internal functions exposed in the public API of mplot3d#13030

anntzer merged 8 commits intomatplotlib:masterfromrth:private-funcs-mplot3d

Feb 12, 2019

Conversation

Copy link

Contributor

rth commentedDec 20, 2018•
edited
Loading

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

rthand others added3 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 fromWeatherGod

December 23, 2018 03:09

QuLogic approved these changes

Dec 23, 2018

View reviewed changes

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

		````````````

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

Copy link

Member

QuLogicDec 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please list them all.

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

Copy link

Member

timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

tacaswell previously requested changes

Dec 24, 2018

View reviewed changes

Copy link

Member

tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@WeatherGod should dismiss this review.

dstansby added the topic: mplot3d label

Dec 26, 2018

WeatherGod reviewed

Jan 7, 2019

View reviewed changes

lib/mpl_toolkits/mplot3d/proj3d.py

		return _proj_transform_vec(vec, M)


		def _proj_transform_vec_clip(vec, M):

Copy link

Member

WeatherGodJan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

WeatherGodJan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

WeatherGodJan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

yeah,line2d_seg_dist() is what is used.

Copy link

Member

WeatherGod commentedJan 7, 2019

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.

Copy link

Contributor

anntzer commentedJan 7, 2019•
edited
Loading

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.

Copy link

Member

WeatherGod commentedJan 7, 2019

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

Copy link

ContributorAuthor

rth commentedJan 7, 2019

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, regardingproj_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 definitionhttps://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?

Copy link

Contributor

anntzer commentedJan 7, 2019•
edited
Loading

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.

tacaswell added this to thev3.1 milestone

Jan 7, 2019

Copy link

Member

WeatherGod commentedJan 7, 2019

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.

Copy link

Contributor

anntzer commentedJan 7, 2019

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

Copy link

Member

WeatherGod commentedJan 7, 2019

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

Copy link

Contributor

anntzer commentedJan 7, 2019

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

tacaswell dismissed theirstale review

January 7, 2019 21:06

addressed.

Copy link

Member

tacaswell commentedJan 7, 2019

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.

Copy link

Member

WeatherGod commentedJan 7, 2019

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 ofline2d_seg_dist() in axes3d.py that was missed. I don't know if there are others.

WeatherGod requested changes

Jan 7, 2019

View reviewed changes

Copy link

Member

WeatherGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Need to find the remaining uses of the deprecated functions.

Copy link

Member

NelleV commentedJan 18, 2019

@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.

anntzer mentioned this pull request

Jan 20, 2019

Vectorize mplot3d.art3d.zalpha.#13235

Merged

6 tasks

rth added3 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

Copy link

ContributorAuthor

rth commentedJan 22, 2019

Thanks for all the feedback.

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

Re-rungrep 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.

tacaswell added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Jan 28, 2019

Copy link

Member

jklymak commentedJan 28, 2019

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

WeatherGod reviewed

Feb 6, 2019

View reviewed changes

lib/mpl_toolkits/mplot3d/axes3d.py

		@@ -1030,7 +1030,7 @@ def get_proj(self):

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

Copy link

Member

WeatherGodFeb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is going to conflict with#13020.

Copy link

Contributor

anntzerFeb 6, 2019•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

WeatherGod reviewed

Feb 6, 2019

View reviewed changes

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

Copy link

Member

WeatherGodFeb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Going to conflict with#13020

WeatherGod reviewed

Feb 6, 2019

View reviewed changes

lib/mpl_toolkits/mplot3d/proj3d.py

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

Copy link

Member

WeatherGodFeb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

efiring commentedFeb 12, 2019

@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?