Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add font size traits to various classes #1899

Merged
merged 1 commit into from Jun 15, 2021

Conversation

23ccozad
Copy link
Contributor

@23ccozad 23ccozad commented Jun 1, 2021

Provides users with the ability to change font sizes for various text elements on maps.

Description Of Changes

  • Add titlesize trait to MapPanel
  • Add clabelsize trait to ContourTraits
  • Add colorbar_fontsize trait to ColorfillTraits
  • Add font_size trait to PlotObs

Checklist

@23ccozad 23ccozad force-pushed the font_size_declarative_interface branch 5 times, most recently from c8a5137 to 0c45837 Compare June 4, 2021 21:17
@kgoebber
Copy link
Collaborator

kgoebber commented Jun 7, 2021

Overall, this PR is looking good. Don't know why it failed on 3.7 minimum requirements...

Thinking a little bit about the naming convention for all of these attributes, I might suggest we go with fontsize (with no underscore for PlotObs and is essentially the standard name), then we can have each thing we set a fontsize for be _fontsize (just as you have for colorbar_fontsize). So that would make the attributes being added in this PR as:

  • fontsize
  • title_fontsize
  • label_fontsize
  • colorbar_fontsize

This would bring a reasonable sense of uniformity to the attribute naming convention and would allow us to expand to other fontsizes as needed in the future using the same convention.

@23ccozad 23ccozad force-pushed the font_size_declarative_interface branch 2 times, most recently from b142d8e to bf82fc7 Compare June 8, 2021 01:48
@kgoebber
Copy link
Collaborator

kgoebber commented Jun 8, 2021

So the 3.7 minimum PyPI tests are failing due to the minimum Matplotlib being 3.0.1. Some quick testing reveals that this issue is resolved in Matplotlib >=3.2 - don't know what our timeline is for bumping the Matplotlib minimum requirement.

In the meantime we can set the tolerance values separately for the minimum requirement. For example, for the test_declarative_colorbar_fontsize test you could modify the decorator to be:

@pytest.mark.mpl_image_compare(remove_text=False,
                               tolerance={'3.1': 11.49,
                                          '3.0': 11.49}.get(MPL_VERSION, 0.607))

The value of 11.5 for each of the matplotlib versions comes from the RMS result when the test fails. Here are the tolerance values for each of the three new tests.

test_declarative_colorbar_fontsize: 11.49
test_declarative_station_plot_fontsize: 2.12
test_declarative_contour_label_fontsize: 20.3

It turns out that there are some small changes in text placement and on the contour label, it changes the location of the labels in the layout, so that results in a larger RMS.

@23ccozad
Copy link
Contributor Author

23ccozad commented Jun 8, 2021

@kgoebber Ryan, Drew, and I came to the same conclusion yesterday, and we'll be moving forward with those changes shortly (on my to-do list today/tomorrow). Then we should be ready for a final review and merge.

@23ccozad 23ccozad force-pushed the font_size_declarative_interface branch from 8ce5d54 to 603eafa Compare June 8, 2021 23:11
@23ccozad 23ccozad marked this pull request as ready for review June 9, 2021 00:12
@23ccozad 23ccozad force-pushed the font_size_declarative_interface branch from 603eafa to d365d87 Compare June 9, 2021 00:15
@23ccozad 23ccozad force-pushed the font_size_declarative_interface branch from d365d87 to a0da8d6 Compare June 9, 2021 14:00
kgoebber
kgoebber previously approved these changes Jun 9, 2021
Copy link
Collaborator

@kgoebber kgoebber left a comment

Choose a reason for hiding this comment

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

This PR looks to be in good order now and is a nice addition to the declarative syntax.

@dcamron dcamron added Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality labels Jun 9, 2021
@dopplershift dopplershift added this to the 1.1.0 milestone Jun 14, 2021
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Nice. Just one style nit and this is ready to go in.

Comment on lines 999 to 1001
self.parent.ax.figure.colorbar(self.handle, orientation=self.colorbar,
pad=0, aspect=50)\
.ax.tick_params(labelsize=self.colorbar_fontsize)
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like Python's line continuation characters. How about:

Suggested change
self.parent.ax.figure.colorbar(self.handle, orientation=self.colorbar,
pad=0, aspect=50)\
.ax.tick_params(labelsize=self.colorbar_fontsize)
cbar = self.parent.ax.figure.colorbar(
self.handle, orientation=self.colorbar, pad=0, aspect=50)
cbar.ax.tick_params(labelsize=self.colorbar_fontsize)

Add titlesize trait to MapPanel. Add clabelsize trait to ContourTraits. Add colorbar_fontsize trait to ColorfillTraits. Add font_size trait to PlotObs.
@dopplershift dopplershift merged commit 3c02587 into Unidata:main Jun 15, 2021
@23ccozad 23ccozad mentioned this pull request Jun 18, 2021
1 task
@23ccozad 23ccozad deleted the font_size_declarative_interface branch June 28, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to change fontsize in declarative interface
4 participants