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

[ENH] Projection plots: Customizable plots #4828

Merged
merged 8 commits into from
Jul 6, 2020

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented May 28, 2020

Issue

Partially implements #4493
Needs biolab/orange-widget-base#74

Description of changes

Enable setting label (axes, labels, legend) fonts, annotations and figure properties on projection widgets and some other widgets with plots

  • Scatterplot, MDS, tSNE, Radviz, Freeviz, LinearProjection
  • Line plot
  • Calibration plot
TODO
  • tick font resize for Calibration plot
  • save settings as schema-only settings
Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT changed the title [ENH] Projection plots: Customize labels [WIP][ENH] Projection plots: Customize labels Jun 5, 2020
@VesnaT VesnaT changed the title [WIP][ENH] Projection plots: Customize labels [ENH] Projection plots: Customize labels Jun 5, 2020
Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

It looks great. I haven't dived into the code yet, I just glanced over it, but haven't caught the entire idea. I'll do it in the following days. Ping me on Monday if I don't.

  • Why can't the user change the font size for ticks?
  • Now that the user can increase it, could we make the default size for labels smaller? 9 looks great on my computer.
  • Do you think it's necessary to have separate font sizes for categorical and numerical legend?

I was too negligent when reviewing biolab/orange-widget-base#70.

  • Reset button resets the graph, but not the values in controls, e.g. spin boxes.
  • Modified settings are not saved. I think they should be saved as schema-only settings.

And now for a killer functionality: a button "Copy settings" (or just "Copy") that would put the settings from the current onto clipboard (not the real one, just a global "orange visual settings clipboard" and a button "Paste Settings" (or "Paste" -- if it's beside "Copy", users will get the idea) that would paste all applicable settings from "clipboard" into the current dialog. It would be totally fancy and probably trivial to implement -- it's almost the same as reset.

Orange/widgets/evaluate/owcalibrationplot.py Outdated Show resolved Hide resolved
@VesnaT
Copy link
Contributor Author

VesnaT commented Jun 8, 2020

Why can't the user change the font size for ticks?

I think the pyqtgraph does not support that (well, it does, but not correctly and the ticks may overlap with the title), so some AxisItem functions should probably be overwritten. I just haven't managed to dive into that yet.

Now that the user can increase it, could we make the default size for labels smaller? 9 looks great on my computer.

If you prefer, sure. Just add self.label_font.setPointSize(9) after self.label_font = QFont().

Do you think it's necessary to have separate font sizes for categorical and numerical legend?

I don't know. I just kept the old functionality (different default sizes). IMO, the smaller size looks better for the numerical one. We can discuss upon that, now that the sizes can easily be changed during the runtime.

Reset button resets the graph, but not the values in controls, e.g. spin boxes.

Strange, it does on my computer. I'll check that.

Modified settings are not saved. I think they should be saved as schema-only settings.

I agree. I thought I'd do it in the next iteration, when the idea is confirmed.

@VesnaT
Copy link
Contributor Author

VesnaT commented Jun 8, 2020

Are you sure about the Reset button? It works fine for me. How about the VisualSettingsDialog itself? You can run it independently of the widget.

I'm always up for the killer functionalities :)

@borondics
Copy link
Member

@markotoplak, is this going to propagate to the Orange Spectroscopy plotting widgets?

@VesnaT VesnaT force-pushed the customize_plots branch 3 times, most recently from 4e97b0b to 4ae30d6 Compare June 10, 2020 08:40
@markotoplak markotoplak changed the title [ENH] Projection plots: Customize labels [NOMERGE][ENH] Projection plots: Customize labels Jun 10, 2020
@markotoplak
Copy link
Member

@borondics, yes.

@VesnaT VesnaT force-pushed the customize_plots branch 4 times, most recently from c9f9d47 to 9175fa9 Compare June 18, 2020 09:26
@VesnaT VesnaT force-pushed the customize_plots branch 2 times, most recently from 8e29156 to dadcb31 Compare June 29, 2020 08:56
@janezd
Copy link
Contributor

janezd commented Jun 30, 2020

This is indeed becoming huge and needs to be reviewed ASAP. It is amazing, though.

Most of the code is in orange-widget-base (which I think is OK). The code here is either "declarative" or takes care of updating. There is not much to check, except that updating indeed updates -- which is to be done manually, I suppose, because part of it is whether there are any pyqtgraph quirks to avoid. Therefore, I think somebody should systematically click through visualizations. Here are comments after my non-systematic clicking:

  • Could the line edit for titles and similar be extended over the entire width of the window?

  • Button OK is misleading; it doesn't confirm the changes (as opposed to Cancel), it just closes the window. Perhaps rename to Close, or you can eliminate it.

  • Is there a reason to include "Antialias"? Would the user ever want to have non-anti-aliased plots?

  • The opacity setting is far from obvious. I saw a spin in line plot, changed it and nothing happenned. Then I checked the tooltip to see what it does. But even after this, I didn't realize that the spins for "Range" and "Selected Range" are also opacities - I had to see the tooltip again. Would it be possible to add a label? Also, could these spins have a step of 5?

  • Fonts start with a list of (exotic) fonts whose names start with "." (on macOS). Well, one of them is the default, San Francisco (shown as .SF). I propose the following ordering:

    • default family (.SF on macOS), but without the leading period.
    • all fonts that don't begin with period.
    • all fonts that begin with period (assuming they are exotic system fonts).

    Here I of course assume that this order is not imposed by Qt and is possible to change. (I haven't checked the code.)

This being a non-essential part of Orange and unlikely to cause any debilitating bugs, I'd vote for merge early and fix as needed.

@ajdapretnar
Copy link
Contributor

I volunteer. I am extremely qualified for systematic clicking. 👑

@ajdapretnar ajdapretnar self-assigned this Jun 30, 2020
@VesnaT
Copy link
Contributor Author

VesnaT commented Jun 30, 2020

Is there a reason to include "Antialias"? Would the user ever want to have non-anti-aliased plots?

Yes. Selection in Line plot is too slow for antialiased plots.

The opacity setting is far from obvious. I saw a spin in line plot, changed it and nothing happenned. Then I checked the tooltip to see what it does. But even after this, I didn't realize that the spins for "Range" and "Selected Range" are also opacities - I had to see the tooltip again. Would it be possible to add a label? Also, could these spins have a step of 5?

I agree with the setting not being obvious, but I'm not sure how to include the label.
The spins do have step of 5.

@janezd
Copy link
Contributor

janezd commented Jun 30, 2020

Yes. Selection in Line plot is too slow for antialiased plots.

OK.

I agree with the setting not being obvious, but I'm not sure how to include the label.

Let's wait if anybody has a suggestion, otherwise leave.

The spins do have step of 5.

I had this idea while writing, and haven't checked. Sorry. :)

@VesnaT
Copy link
Contributor Author

VesnaT commented Jun 30, 2020

Button OK is misleading; it doesn't confirm the changes (as opposed to Cancel), it just closes the window. Perhaps rename to Close, or you can eliminate it.

Renamed in biolab/orange-widget-base#83

@VesnaT
Copy link
Contributor Author

VesnaT commented Jun 30, 2020

Fonts start with a list of (exotic) fonts whose names start with "." (on macOS). Well, one of them is the default, San Francisco (shown as .SF). I propose the following ordering:
-default family (.SF on macOS), but without the leading period.
-all fonts that don't begin with period.
-all fonts that begin with period (assuming they are exotic system fonts).
Here I of course assume that this order is not imposed by Qt and is possible to change. (I haven't checked the code.)

I can change the order (Qt provides a sorted list of font families), but I'd rather not remove the period. If I removed it, I'd need a mapping (back to original) which would complicate the code.

@VesnaT VesnaT changed the title [NOMERGE][ENH] Projection plots: Customize labels [NOMERGE][ENH] Projection plots: Customizable plots Jul 3, 2020
@VesnaT VesnaT changed the title [NOMERGE][ENH] Projection plots: Customizable plots [ENH] Projection plots: Customize labels Jul 3, 2020
@ajdapretnar
Copy link
Contributor

Works great so far! :)

@VesnaT VesnaT changed the title [ENH] Projection plots: Customize labels [ENH] Projection plots: Customizable plots Jul 3, 2020
@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #4828 into master will decrease coverage by 0.08%.
The diff coverage is 78.46%.

@@            Coverage Diff             @@
##           master    #4828      +/-   ##
==========================================
- Coverage   84.16%   84.08%   -0.09%     
==========================================
  Files         282      278       -4     
  Lines       57465    56991     -474     
==========================================
- Hits        48368    47923     -445     
+ Misses       9097     9068      -29     

@ajdapretnar ajdapretnar merged commit c11ec5e into biolab:master Jul 6, 2020
@ajdapretnar
Copy link
Contributor

We decided to ignore coverage, because tests would take months to write and in the meantime this PR would get outdated. Wrote an issue #4891 not to forget about it.

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.

5 participants