wiheto / netplotbrain

A package to create simple 3D network visualizations on a brain.
Apache License 2.0
67 stars 7 forks source link

Adding title and subtitle behaviour to figures. Improving #28 by seprating title from subtitle #32

Closed silviafan closed 2 years ago

silviafan commented 2 years ago

Fixed bug #28

wiheto commented 2 years ago

I don’t fully understand what this function is doing and seems to be unnecessarily repeating a lot of code from other functions.

Could you please comment the code and also check it doesn’t break behaviour we do not want to change.

Regarding commenting… At the moment, I do not understand what the “view” argument is in titlealign (the logic may be better than I conceived, I just need it explained).

Regarding checking the functionality… I am unsure if the way it is expressed if this breaks the behaviour we want when you may want multiple titles in a figure.

28 was about not having repeating titles when there should only be one, not against having multiple titles.

wiheto commented 2 years ago

So to clarify, I don’t think this is the wrong approach. But:

  1. I don’t think you need a new function that repeats the add_subtitle function. You should amend the original function.

  2. The problem is when the same title repeats over multiple subplots, not that we should make it so titles only appear in one subplot (that’s what this is doing I think). This should also be automatically be detected, not rest on input from the user to specify a textalign argument. That’s an additional argument to help control the behaviour.

  3. That the view argument can help text align may work. However, it is possible that users specify their own rotation/elevation arguments instead of default views or use the frames argument function and then they wouldn’t be able to use textalign. So that behaviour would have to be added here. But perhaps the subplot index may be a better way for a user to control this.

silviafan commented 2 years ago

Thanks for the feedback, William. I have edited the code and slimmed it down a lot. The logic is now the following:

  1. If specified, the argument 'title' will now assign the chosen title to the entire figure and is not going to be repeated over multiple subplots anymore.

  2. The title of each single subplot can now be controlled via the 'subtitles' argument. If 'subtitles' is specified, the user can also assign a title to each single subplot.

I have commented the code I have added, but wanted to explain the overall logic here too.

wiheto commented 2 years ago

Let me know if any of these changes are unclear.

wiheto commented 2 years ago

2 further suggestions (or one suggestion and one "watch out for"):

  1. Also, perhaps consider adding following kwargs (to default.json and api.md): titlehorizontalalignment (default center) and titleverticalalignment (default top) and then add fig.suptitle(title, va=profile['titlevertialalignment'], ha=profile['titlevertialalignment']). See: https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.suptitle.html. This will add flexibility about where the main title goes.

  2. When running the example file with these changes, does any text disappear (especially when the figures legends are present). We may need to add constrained_layout command. See example here: https://matplotlib.org/stable/tutorials/intermediate/constrainedlayout_guide.html#sphx-glr-tutorials-intermediate-constrainedlayout-guide-py But since we do not have xlabels/ylabels, this may not be a problem. Just be on the lookout when testing that nothing gets "lost" from the figure. I think it is ok.

silviafan commented 2 years ago

Here is the logic for the ”title” and ”subtitles” arguments

  1. If title is specified, title goes in the top center of the figure
  2. If title is not specified, no title (blank)

Regardless of the title choice:

  1. If figure has only one subplot: 0.a. If title is specified, title goes in the top center of the figure and there are no subtitles 0.b If title is not specified, the pre-specified viewing angle is shown
  2. If the user wants the pre-specified viewing angles to be shown, "subtitles" does not need to be specified (default='auto')
  3. If the user does not want the subtitles, subtitles can be turned off (subtitles=None)
  4. If the user wants to customize subtitles, "subtitles" needs to be a list containing as many strings as the number of subplots in the figure
wiheto commented 2 years ago

Resolved the conflicts because of the updates.

But the keyword arguments to customize: titlefontsize, titleloc, titleweight now modify the subtitle, not title.

I think these need to be changed to subtitlefontsize, subtitleloc, subtitleweight, in all the relevant places (default.json, ./docs/api.md and _add_subplot_title)

And I think the title should be able to be modified with titlefontsize, titleloc, titleweight.

wiheto commented 2 years ago

So I merged these and will fix the small adjustments relating to the keyword arguments