Multiple plotting GUI #24338

AnthonyLim23 · 2018-12-14T11:41:38Z

Description of work.
The multiple plotting in elemental analysis was not easily extendable or maintainable. This PR is the first stage of rewriting the functionality into an easier to maintain and extend form. I will add the test in a later PR

Report to: [user name]/[nobody].

To test:
In the Framework/Properties/Mantid.properties.template file you will have to add Muon/plotting_test.py to the list of interfaces and then build Mantid.

Open the interface
play with it.
The combobox at the bottom determines which plot(s) you change when editing the x and y ranges, errors etc.

Fixes #24036 and #24341 and #23558.

Reviewer

Please comment on the following (full description):

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there are changes in the release notes then do they describe the changes appropriately?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Matthew-Andrew · 2018-12-14T12:02:05Z

scripts/MultiPlotting/QuickEdit/quickEdit_model.py

+# SPDX - License - Identifier: GPL - 3.0 +
+from __future__ import absolute_import, print_function
+
+from MultiPlotting.multiPlotting_context import *


I have a dislike of wildcard imports as it makes it more difficult to work out what comes from where. Could we change this to an explicit import?

I will try to get the constants directly.

Matthew-Andrew · 2018-12-14T12:49:23Z

scripts/MultiPlotting/AxisChanger/axis_changer_presenter.py

+#     & Institut Laue - Langevin
+# SPDX - License - Identifier: GPL - 3.0 +
+
+class AxisChangerPresenter(object):


Is this class strictly needed it seems to just simply forward everything to the view?

it would be useful if we ever decide to restrain acceptable inputs. We could then do the logic in here. However, yes it is unneeded for this case.

Matthew-Andrew · 2018-12-14T12:51:06Z

scripts/MultiPlotting/QuickEdit/quickEdit_model.py

+
+# use axis changer object later
+
+class QuickEditModel(object ):


Is what this model stores ever used? I found replacing it with a mock.MagicMock didn't seem to have an effect

Matthew-Andrew · 2018-12-14T12:56:08Z

scripts/MultiPlotting/QuickEdit/quickEdit_widget.py

+from MultiPlotting.QuickEdit.quickEdit_model import QuickEditModel
+from MultiPlotting.QuickEdit.quickEdit_presenter import QuickEditPresenter
+
+class QuickEditWidget(object):


Is this class needed ? as it seems to mainly forward all of it's methods straight to methods on the presenter with the same names.

This is something I have to clean up the code at import. It is a wrapper so we only have to import this if we want to use the widget. As opposed to importing the view, presenter and model.

Matthew-Andrew · 2018-12-14T13:02:02Z

scripts/MultiPlotting/navigation_toolbar.py

+        self.layout().takeAt(5)  # or more than 1 if you have more buttons
+        pm = QtGui.QPixmap()
+        ic = QtGui.QIcon(pm)
+        # self.add = self.addAction(ic, "Add line")


stray comments

these are intentional as it is the coded we need to enable the remove line button in the toolbar. I just have not moved the code over yet.

Matthew-Andrew · 2018-12-14T13:13:25Z

scripts/MultiPlotting/subplot/subPlot.py

+    def set_plot_y_range(self,subplotNames,range):
+        for subplotName in subplotNames:
+            self.plotObjects[subplotName].set_ylim(range)
+            #self._context.subplots[subplotName].ybounds =range 


stray comment

Matthew-Andrew · 2018-12-14T13:29:51Z

scripts/MultiPlotting/navigation_toolbar.py

+    def __init__(self, *args, **kwargs):
+        super(myToolbar, self).__init__(*args, **kwargs)
+        self.layout().takeAt(5)  # or more than 1 if you have more buttons
+        pm = QtGui.QPixmap()


Does this and the line below do anything?

that should be commented out too

Matthew-Andrew · 2018-12-14T13:35:38Z

scripts/MultiPlotting/multiPlotting_presenter.py

@@ -0,0 +1,38 @@
+# Mantid Repository : https://github.com/mantidproject/mantid


Is this class used anywhere?

I will remove it

Matthew-Andrew · 2018-12-14T13:37:05Z

scripts/MultiPlotting/multiPlotting_model.py

@@ -0,0 +1,23 @@
+# Mantid Repository : https://github.com/mantidproject/mantid


Is this class used anywhere?

dont think so, I will kill it

Matthew-Andrew · 2018-12-14T13:41:44Z

scripts/MultiPlotting/multiPlotting_view.py

@@ -0,0 +1,36 @@
+# Mantid Repository : https://github.com/mantidproject/mantid
+#


Is this class used anywhere?

I will remove it

AnthonyLim23 · 2018-12-14T15:36:51Z

I have decided to add the tests to this branch so I have marked it as in progress.

AnthonyLim23 · 2018-12-19T16:00:26Z

unrelated failure

mantid-builder · 2019-01-03T14:03:16Z

@AnthonyLim23 have you had a chance to see why the build is failing?

Matthew-Andrew

Looks good 👍 I have one minor comment in that when the tick button is pressed to bring up the customization window the drop down menu lists the plots by a name which includes the memory address. I think it would be clearer if this were just the plot title?

AnthonyLim23 · 2019-01-08T11:48:29Z

I have noticed that too. Since it is a matplotlib thing I am not sure how to fix it

Matthew-Andrew · 2019-01-09T09:23:15Z

Ok as its a minor point and difficult to fix happy to leave for now.

AnthonyLim23 added 12 commits November 19, 2018 16:59

refs #24036 basic multi plotting has been refactored

703e9d1

refs #24036 removed unneeded files for multi plotting

ee2da0a

refs #24036 subplot GUI now has x,y controls

bdfb788

refs #24036 subplot GUI now has working error tick and autoscale

ca4d639

refs #24036 plot selector in multi plotting populated

8d152fb

refs #24036 autoscale and zoom (except with errors) in multi plotting

9060d5f

refs #24036 multi plotting bug fix for errors

d433ee7

refs #24036 auto scale works with multi plot

30bd6f4

refs #24036 auto scale works for multi plotting

0199375

refs #24036 added autocomplete box to multi plot

614149f

refs #24036 merge master

40b8080

refs #24036 restored elemental analysis

e48d3da

AnthonyLim23 added the Component: GUI label Dec 14, 2018

AnthonyLim23 added this to the Release 3.14 (4.0?) milestone Dec 14, 2018

AnthonyLim23 assigned Matthew-Andrew Dec 14, 2018

refs #24036 flake8 fixes for multiplotting

3813c14

AnthonyLim23 mentioned this pull request Dec 14, 2018

tests for multi plotting #24341

Closed

Matthew-Andrew reviewed Dec 14, 2018

View reviewed changes

AnthonyLim23 added 4 commits December 14, 2018 14:50

refs #24036 rm unused code from multiplot

a9f1755

refs #24036 rm unused code from multiplot

9142174

refs #24036 rm unused code from multiplot - flake8

57b63e3

refs #24036 fixed flake8 on multiplot

240bff7

AnthonyLim23 added the State: In Progress label Dec 14, 2018

AnthonyLim23 added 2 commits December 17, 2018 13:03

refs #24036 added qucikedit tests v1

8c387de

refs #24036 unit tests for multi plotting

f9d93f2

AnthonyLim23 removed the State: In Progress label Dec 19, 2018

refs #24036 made multiplotwidget test python3 compatible

1c32fcb

Matthew-Andrew reviewed Jan 8, 2019

View reviewed changes

Matthew-Andrew approved these changes Jan 9, 2019

View reviewed changes

martyngigg mentioned this pull request Jan 14, 2019

Tiling matplotlib plots #24486

Closed

SimonHeybrock merged commit 4e2cc75 into master Jan 15, 2019

SimonHeybrock deleted the 24036_refactorMuonPlotting branch January 15, 2019 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple plotting GUI #24338

Multiple plotting GUI #24338

AnthonyLim23 commented Dec 14, 2018 •

edited

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

Matthew-Andrew Dec 14, 2018

AnthonyLim23 Dec 14, 2018

AnthonyLim23 commented Dec 14, 2018

AnthonyLim23 commented Dec 19, 2018

mantid-builder commented Jan 3, 2019

Matthew-Andrew left a comment

AnthonyLim23 commented Jan 8, 2019

Matthew-Andrew commented Jan 9, 2019


		# use axis changer object later

		class QuickEditModel(object ):

		@@ -0,0 +1,38 @@
		# Mantid Repository : https://github.com/mantidproject/mantid

		@@ -0,0 +1,23 @@
		# Mantid Repository : https://github.com/mantidproject/mantid

		@@ -0,0 +1,36 @@
		# Mantid Repository : https://github.com/mantidproject/mantid
		#

Multiple plotting GUI #24338

Multiple plotting GUI #24338

Conversation

AnthonyLim23 commented Dec 14, 2018 • edited

Reviewer

Code Review

Functional Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLim23 commented Dec 14, 2018

AnthonyLim23 commented Dec 19, 2018

mantid-builder commented Jan 3, 2019

Matthew-Andrew left a comment

Choose a reason for hiding this comment

AnthonyLim23 commented Jan 8, 2019

Matthew-Andrew commented Jan 9, 2019

AnthonyLim23 commented Dec 14, 2018 •

edited