Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[ENH]: Use new style format strings for colorbar ticks#21542

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

Merged
jklymak merged 2 commits intomatplotlib:mainfromvfdev-5:fix-21378
Nov 9, 2021

Conversation

vfdev-5
Copy link
Contributor

@vfdev-5vfdev-5 commentedNov 4, 2021
edited
Loading

Fixes#21378

PR Summary

This PR introducesformatter as property to fetch axis format and thus new style string formatter is used
If applying a similar logic to major and minor locators, we can end up with removing the code fromupdate_ticks.
Tricky part is withself.norm.
If we are ok with this solution, I can work on doing the same forlocator andminorlocator.

Context: PR from PyData Global 2021 mentored sprint

cc@tacaswell

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@tacaswelltacaswell added this to thev3.6.0 milestoneNov 4, 2021
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

There are two things to watch out for:

  • A hard change form %-style to to new-style for the theformat parameter would be a breaking change. We cannot do that because users would have to stictly adapt their code with the changed Matplotlib version, which would be really annoying.

    What you have to do is accept new style format strings along-side %-style.

  • Mapping formatter by a property is a separate topic. I suggest to follow up on this in a separate PR if you are interested. Deduplicating state by using a property instead of holding the formatter inself.formatter and in the long axis seems a reasonable approach, however there are some things to watch out for:

    • You'll loose the formatter with your approach if you switch the orientation. I'm not quite sure if switching the orientation works at all, but if so that's a de-facto feature that we have to continue to support.

    • As implemented here, this would allow for code like

      >>> cbar.formatter = "{x:.3f}">>> cbar.formatter<StrMethodFormatter>

      This asymmetry is a somewhat unexpected behavior for a property, and I'm not sure, we want to allow that.

    • The same mechanism is used for locator and minorlocator. If we switch to properties all three should be changed together.

vfdev-5 reacted with thumbs up emoji
@jklymak
Copy link
Member

The long-term goal is to make colorbar axises as similar as possible to normal axises. Ideally colorbar.locator and colorbar.formatter would just be dumb reflections of what is on the long axis.

vfdev-5 reacted with thumbs up emoji

@vfdev-5
Copy link
ContributorAuthor

@timhoffm thanks for the review! Yes, I agree that the way it is done here is unaffordable breaking change.

What you have to do is accept new style format strings along-side %-style.

So, the idea is to revert the changes and do something like

ifisinstance(format,str):# Check format type between FormatStrFormatter and StrMethodFormattertry:self.formatter=ticker.FormatStrFormatter(format)_=self.formatter(0)exceptTypeError:self.formatter=ticker.StrMethodFormatter(format)else:self.formatter=format# Assume it is a Formatter or None

?

Mapping formatter by a property is a separate topic. I suggest to follow up on this in a separate PR if you are interested.

Yes, I can send other PRs with that however I do not quite understand the whole picture of what it can break etc (some guidance could be needed).

You'll loose the formatter with your approach if you switch the orientation. I'm not quite sure if switching the orientation works at all, but if so that's a de-facto feature that we have to continue to support.

Could you please explain what it the switch of the orientation (with some code if possible) ? Thanks

The long-term goal is to make colorbar axises as similar as possible to normal axises.

Yes, Thomas said something like that during the mentored sprint and that's why I went using property.

@timhoffm
Copy link
Member

So, the idea is to revert the changes and do something like ...

Yes.

Could you please explain what it the switch of the orientation (with some code if possible)

I thought you could swich the orienation by assigning toColorbar.orientation

from matplotlib.ticker import PercentFormatterplt.imshow(np.random.random((3, 3)))cb = plt.colorbar(format=PercentFormatter(), orientation='vertial')cb.orientation = 'horizontal'

but apparently, this does not have any effect.

Yes, I can send other PRs with that however I do not quite understand the whole picture of what it can break etc (some guidance could be needed).

Currently, Colorbar holds the state and pushes the information down to the respective axis when needed; e.g.

self._long_axis().set(label_position=self.ticklocation,
ff.

This is safe against operations like changing orientation (i.e. long and short axis) or clearing of an axis (I don't know that is happens, but would be ok in the current scenario). If the axis holds the state instead, we have to make sure that it's really kept consistent, which could get messed up if long and short axis are exchanged.
I don't know if any of these are practical problems, but that needs to be investigated if we move the responsibility for the state.

vfdev-5 reacted with thumbs up emoji

@jklymak
Copy link
Member

First, I think changing the orientation of a colorbar after creating it is a non-starter, and certainly not something we have done previously.cb.orientation should probably be read-only somehow, but...

For this PR, we have two separate issues that we are looking at:

  1. if the user passes "format" to the instantiation, it could accept the "new" formatting specifier: I think this is fine, but we can't stop accepting the old format specifier

  2. setter and getter for thecb.format property. This seems helpful in that it is getting and setting from the long_axis. However, I wonder if we also want to rename this so that it is clear that we are really doingcb.set_major_formatter? Or trying to discourage operating on the colorbar directly at all. Where I get confused and have to look carefully each time, is how much formatter/locator logic needs to sit at the colorbar level because of the norm. ie. do we even want users doingcb._long_axis.set_major_formatter()?

Given that these are pretty orthogonal, I agree with@timhoffm that this should maybe be made into two PRs, with 1) being a lot easier than 2)!

vfdev-5 reacted with thumbs up emoji

both StrMethodFormatter and FormatStrFormatter if format is str
@vfdev-5
Copy link
ContributorAuthor

@timhoffm@jklymak thanks for the details and the info. I reverted the code and added as proposed in a previous message a way to switch between 2 formatters if input format isstr. Updated also the tests. Please let me know if this is sufficient for the PR "1". Thanks!

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Module fixing flake8.

vfdev-5 reacted with thumbs up emoji
@jklymakjklymak merged commita3ba7cf intomatplotlib:mainNov 9, 2021
@jklymak
Copy link
Member

Thanks@vfdev-5 ! And thanks for participating in the sprint!

vfdev-5 reacted with hooray emoji

@vfdev-5vfdev-5 deleted the fix-21378 branchNovember 9, 2021 10:40
@vfdev-5
Copy link
ContributorAuthor

@jklymak I'd like to provide a follow-up PR with setter/getter property forcb.format and maybe other attributes. Should we discuss the scope somewhere in a new issue or I can send a draft PR and we can affine it there ?

@jklymak
Copy link
Member

I usually prefer a draft PR, even if its just a straw man for discussion, so long as it is not too much work

vfdev-5 and timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

This should still get a what's new entry.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@jklymakjklymakjklymak approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[ENH]: use new style format strings for colorbar ticks
4 participants
@vfdev-5@jklymak@timhoffm@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp