Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Deprecate position argument to Axes.apply_aspect()#23629
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
35a21f9 tod0584cdCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I think if you search on stackoverflow there are quite a few answers that suggest calling apply_aspect to get the position of things after the proper aspect ratio is applied without forcing a full draw. |
Well, maybe only a couple:https://stackoverflow.com/search?q=%5Bmatplotlib%5D+apply_aspect |
timhoffm commentedAug 18, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
From these:
Anyway, let's be super-safe and only deprecate the publicposition parameter for now. - No end user should manipulate the position through |
d0584cd to9a68b94Compare9a68b94 tobdcaa9cComparePlease update the commit message as well for that. |
7784ff2 to38fbaadCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've found a use ofapply_aspect in the wild that this rename would likely break without the deprecation triggering
https://github.com/AudioSceneDescriptionFormat/splines/blob/master/doc/rotation/helper.py#L275
I'm not entirely sure what they are attempting to do there.
| @@ -0,0 +1,3 @@ | |||
| The parameter *position* in ``Axes.apply_aspect()`` | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
| ... is deprecated. It is considered internal API and now public use case is known. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ... is deprecated. It is considered internal API andnow public use case is known. | |
| ... is deprecated. It is considered internal API andno public use case is known. |
lib/matplotlib/axes/_base.py Outdated
| position : None or .Bbox | ||
| If not ``None``, this defines the position of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| position :Noneor .Bbox | |
| Ifnot``None``,thisdefinesthepositionofthe | |
| position :Noneor .Bbox | |
| Ifnot``None``,thisdefinesthepositionofthe |
One use of And one that passes a position:https://github.com/astropy/astropy/blob/main/astropy/visualization/wcsaxes/core.py#L444-L452 |
timhoffm commentedAug 19, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Deferred to 3.7. Let’s not rush this into 3.6. This needs a more thorough investigation. |
38fbaad to57c4ddfCompareDigging through the history my understanding of the reason for the position parameter is the following: It is somewhat surprising that "_apply_aspect" takes an optional position as input, which seems more functionality than what the name suggests. The parameter was introduced in66290aae. Generally, applying an aspect will modify the size and position of an Axes. The fact that position is updated anyway was used to funnel additional position information from a layoutinto the already existing positioning code. IMHO:
I see two ways forward:
I'm inclined to go with 1.
has meanwhile been changed to a draw. 👍 |
Superseedsmatplotlib#23629.It is somewhat surprising that "_apply_aspect" takes anoptional position as input, which seems more functionalitythan what the name suggests.The parameter was introduced in66290aa. Generally, applyingan aspect will modify the size and position of an Axes. Thefact that position is updated anyway was used to funneladditional position information from a layout into the alreadyexisting positioning code.Deprecating and removing the parameter would be a medium compatibilityhassle. Therefore, I chose only to document the status quo and itsintention, so that users are less surprised. Seematplotlib#23629 (comment)
Superseedsmatplotlib#23629.It is somewhat surprising that "_apply_aspect" takes anoptional position as input, which seems more functionalitythan what the name suggests.The parameter was introduced in66290aa. Generally, applyingan aspect will modify the size and position of an Axes. Thefact that position is updated anyway was used to funneladditional position information from a layout into the alreadyexisting positioning code.Deprecating and removing the parameter would be a medium compatibilityhassle. Therefore, I chose only to document the status quo and itsintention, so that users are less surprised. Seematplotlib#23629 (comment)
Superseedsmatplotlib#23629.It is somewhat surprising that "_apply_aspect" takes anoptional position as input, which seems more functionalitythan what the name suggests.The parameter was introduced in66290aa. Generally, applyingan aspect will modify the size and position of an Axes. Thefact that position is updated anyway was used to funneladditional position information from a layout into the alreadyexisting positioning code.Deprecating and removing the parameter would be a medium compatibilityhassle. Therefore, I chose only to document the status quo and itsintention, so that users are less surprised. Seematplotlib#23629 (comment)
Superseedsmatplotlib#23629.It is somewhat surprising that "_apply_aspect" takes anoptional position as input, which seems more functionalitythan what the name suggests.The parameter was introduced in66290aa. Generally, applyingan aspect will modify the size and position of an Axes. Thefact that position is updated anyway was used to funneladditional position information from a layout into the alreadyexisting positioning code.Deprecating and removing the parameter would be a medium compatibilityhassle. Therefore, I chose only to document the status quo and itsintention, so that users are less surprised. Seematplotlib#23629 (comment)
Not going forward with the deprecation. The parameter is now documented as internal and that's good enough. |
PR Summary
In itself
apply_aspect()feels too low level to be needed by an end-user. I'm not sure if it reasonably works at all on its own. It might be that the result is influenced by other artists and if something is left stale, then the application result is still not final. In that case, one would need a full draw (without rendering) anyway. OTOH ifapply_aspect()was really standalone, I don't know why we defer the application and don't apply it immediately when setting the aspect.Also, theposition parameter of
apply_aspectdoes not seem a user-facing API. It is used internally by layout mechanisms. But shifting the position explicitly as part of aspect application seems overreach of the scope of the function and at least not something a user should do.I'm 99% percent sure
apply_aspect()does not need to be public API, but please speak up if you think otherwise. And even if we miss a justified use-case for a user. They should be able to give feedback during the deprecation period and we would reconsider then.