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

Reintroduce title parameter #98

Closed
wants to merge 1 commit into from
Closed

Conversation

StefRe
Copy link

@StefRe StefRe commented Feb 15, 2021

Partly revert 205e6c7: set title if a title is provided, otherwise use ylabel as title.

IMO, the title should convey the main meaning of the chart. If you just want to compare performance of different kernels in a single chart it makes sense to move the only relevant information from the ylabel to the title. If you want to compare performance for different scenarios, however, i.e. if there's another parameter besides the one on the x axis, it's necessary to put this additional parameter in the title of the chart and hence leave the ylabel at the axis, see for instance here. Of course you can fix it after the plot as in lines 57-58 here but it would be much easier to have to possibility to call perfplot with the desired parameters.

If you don't supply a title nothing changes as compared to the current behavior, so it's fully backwards compatible. If you provide a title it'll be set as the axes title and the default title will become the y label.

I didn't understand how you run your pytests - for me (my default backend is Qt5Agg) they open plot windows that have to be closed manually for the tests to carry on, so I wasn't able to run them automatically. In the two tests I added I used my usual approach for this, not sure if you like it (here I described an alternative).

Partly revert 205e6c7: set title if a title is provided, otherwise use ylabel as title
@StefRe
Copy link
Author

StefRe commented Feb 15, 2021

also I noticed that there's a warning when you run test_save after test_automatic_scale:

perfplot_test.py::test_save
perfplot/perfplot/main.py:142: UserWarning: Attempted to set non-positive bottom ylim on a log-scaled axis.
Invalid limit will be ignored.
plt.gca().set_ylim(bottom=0)

The (most probable) reason is that the current figure is not cleared after test_automatic_scale. I think the best way to fix it is to use plt as a fixture and properly tear it down after each test. I didn't fix it as it's in code I didn't touch and it's just a warning in the end.

@nschloe
Copy link
Owner

nschloe commented Feb 16, 2021

I don't like adding styling to perfplot as this unnecessarily moves some styling code into the library. As you correctly point out, one can already do this outside of perfplot with

ax.set_ylabel(ax.get_title())

If we would add this change, the next thing that'll happen is that someone else will want her title in green, so there's another title_color option, and so forth.

The reason why I don't do y-axis titles anymore is because I think they are bad UI. When writing text, the reader always has to turn her head. This can't be it. It possible, I rotate the axis label, but for Runtime [s] this consumes too much horizontal space. A better solution would be to rotate and move the ylabel above the y-axis (in dufte), and to leave the title open.

@nschloe nschloe closed this Feb 16, 2021
@StefRe
Copy link
Author

StefRe commented Feb 16, 2021

Well, I wasn't talking about styling but rather about content and I gave a use case where we need a meaningful title to compare different plots but okay, it's your decision.

@nschloe
Copy link
Owner

nschloe commented Feb 18, 2021

Once there is a solution for matplotlib/matplotlib#19034, we can think about moving the title contents back to the yaxis. Till then, you can use suptitle or the workaround you provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants