Skip to content

Refactor plotters#1058

Open
oerc0122 wants to merge 9 commits into
ISISNeutronMuon:protosfrom
oerc0122:cleanup-plotter
Open

Refactor plotters#1058
oerc0122 wants to merge 9 commits into
ISISNeutronMuon:protosfrom
oerc0122:cleanup-plotter

Conversation

@oerc0122

@oerc0122 oerc0122 commented Nov 7, 2025

Copy link
Copy Markdown
Collaborator

Description of work

  • Refactor all the plotters to use generator constructs, gridspecs and attempt to reduce code-duplication.
  • Adds new "grouped" plot type with one plot per dataset.

Fixes
Prevent number of plots corresponding to number of datasets.

To test
Try to break plotting with weird sequences of button presses.

@oerc0122 oerc0122 self-assigned this Nov 7, 2025
@oerc0122 oerc0122 added the Technical Debt Legacy code which should be cleaned up. label Nov 7, 2025
@MBartkowiakSTFC

Copy link
Copy Markdown
Collaborator

I have been testing the plotter. What I found so far is:

  1. Grid plots of 1D datasets have an empty legend.
  2. If I plot into Grid or Single with legend switched off, and switch it on afterwards, I get an exception: <class 'AttributeError'> 'NoneType' object has no attribute 'set_visible'
  3. Heatmap plot is labelling the plot axes wrong. In my test, time was shown as Q and Q was shown as time.
Screenshot 2025-11-10 at 11 51 59
  1. Heatmap plots start off without any legend. If I switch legend off and on, the legend appears.
  2. If I try to plot a vector summary from the plot creator like this
Screenshot 2025-11-10 at 12 07 03

the plot type ends up being Single and not Vectors:

Screenshot 2025-11-10 at 12 06 42

While we do not want to allow Vectors plot type for normal data sets, it should still be possible to use Vectors for vectors.

  1. For Single and Heatmap plots, switching the legend off and on makes all the subplots resize.
    Before:
Screenshot 2025-11-10 at 12 18 02

After:
Screenshot 2025-11-10 at 12 18 12

@oerc0122 oerc0122 force-pushed the cleanup-plotter branch 2 times, most recently from 017e4f3 to 77472d1 Compare November 19, 2025 13:27
@oerc0122 oerc0122 force-pushed the cleanup-plotter branch 13 times, most recently from 1a65a63 to 0e710ae Compare June 9, 2026 09:34
@MBartkowiakSTFC

Copy link
Copy Markdown
Collaborator

I spent some time plotting data using this branch. A lot of it works already.
I think that these points would need to be looked at:

  1. I managed to make the Heatmap plot fail by changing the main axis of one of the 3 subplots:
Screenshot 2026-06-09 at 11 19 39 Similarly, unchecking the "Use it" checkbox for one of the datasets makes the entire `Heatmap` (or `Grouped`) plot fail.
  1. When I got the Heatmap plot of DISF to appear using the Q vector as the main axis, the plot appears reversed. The signal should become broader in energy at large Q values, but here it is the other way around:
Screenshot 2026-06-09 at 11 23 25
  1. In Single plots, the legend for each curve used to include information on the position of each curve on the other axis. That is, this DISF plot would normally have a legend telling me that these curves correspond to different Q values:
Screenshot 2026-06-09 at 11 27 01 `Grid` and `Grouped` plots in this branch do it correctly and show the q position of each cut.
  1. After switching the main axis there and back (to Q and to energy again) and going through Single, Grid, Grouped plotters, when I go back to Heatmap I get a plot where both axes are the energy axis:
Screenshot 2026-06-09 at 11 31 15

@MBartkowiakSTFC MBartkowiakSTFC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it would make sense to merge this PR once the linter is happy with it.

Normalisation of heatmap plots will require more work and will be best dealt with in a separate PR.
The only other detail is that the first plot (available vs. selected vectors per shell) in vector statistics is now a histogram (and it used to show a point per shell). However, the values are still correct. We can always decide at a later time which plotting style works the best.

@ChiCheng45

Copy link
Copy Markdown
Collaborator

Some issues with the plotter, 3D plotter, and resizing.

Animation43

@MBartkowiakSTFC

Copy link
Copy Markdown
Collaborator

Some issues with the plotter, 3D plotter, and resizing.

OK, you're right. It can happen on my computer too, if I shrink the GUI window enough.

In case it helps, my first guess is that it has something to do with the line self._figure.set_layout_engine("tight") which has been commented out in this PR.

@oerc0122

oerc0122 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

This issue is actually due to the new length of the legend. I am going to also make long legend labels collapse with a ... which seems reasonable.

@MBartkowiakSTFC

Copy link
Copy Markdown
Collaborator

This issue is actually due to the new length of the legend. I am going to also make long legend labels collapse with a ... which seems reasonable.

This may be the case. Still, you can create a legend larger than the plot itself also in the protos branch, and there the plot does not get resized.

On my machine I found that the minimal change to this branch that eliminates this specific problem is to remove the layout keyword argument from PlotWidget line 384
mpl.figure(layout="constrained")
and just create a default figure instead
mpl.figure()
But this leads to other problems later when switching to Heatmap and back, since Heatmap changes the matplotlib layout engine at the moment. There could be other consequences in other plot types too...

@ChiCheng45

ChiCheng45 commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

On the single plot, the number of plots is limited to a small number of line plots per dataset. Can you also do this from the grouped plots? There are some issues if you have a lot of line plots and you use the Y offset.

Here I have 441 line plots. When Y offset is used, it becomes quite messy.

Animation44

Also, on the grouped plot, the home button doesn't work as expected compared to single. On single plot, the home button resets the plot so it fills the space; this doesn't always happen on grouped.

Animation45

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

Labels

Technical Debt Legacy code which should be cleaned up.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants