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.

Sign up for GitHub

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

Jump to bottom

Ess gui 1574 invariant #1590

Merged
merged 31 commits into from
Jul 6, 2020
Merged

Ess gui 1574 invariant #1590

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
23968a7
Initial Invariant update from 4.x PR
butlerpd Jun 8, 2020
f7b872d
Second stage of invariant update port from 4.x
butlerpd Jun 8, 2020
cba0e10
Final changes needed to port 4.x Invariant upgrades to 5.x
butlerpd Jun 14, 2020
7618493
Merge remote-tracking branch 'origin/ESS_GUI' into ESS_GUI_1574_Invar…
butlerpd Jun 14, 2020
af992c4
Remove line feed from invariant_help.rst
butlerpd Jun 14, 2020
dc18842
Add new 1D test data sets
butlerpd Jun 28, 2020
53de045
change Sv code based on review comments
butlerpd Jun 29, 2020
11e3a99
Removing note on factor of 2 from documentation.
butlerpd Jun 29, 2020
ea89300
Merge remote-tracking branch 'origin/ESS_GUI' into ESS_GUI_1574_Invar…
butlerpd Jun 29, 2020
024a434
Fixing failing Status when error occurs
Jun 30, 2020
992f9f2
Changing default contarst so 1000A_sphere_dsm.xml exmaple can work ou…
Jul 1, 2020
48041a6
updating screenshots
Jul 1, 2020
369d9da
Merge branch 'ESS_GUI_1574_Invariant' of https://github.com/SasView/s…
Jul 1, 2020
40b6d20
rescaling images
Jul 1, 2020
5940cbd
Fix typos and failing default calculation reviewing PR1590
Jul 1, 2020
e636f16
Fxining lint issues
Jul 1, 2020
efcef1d
Fixed faulty note and implict ref therein.
Jul 1, 2020
89741d4
Fixing SLD contrast unit
Jul 1, 2020
c1080fe
Merge branch 'ESS_GUI_1574_Invariant' of https://github.com/SasView/s…
Jul 1, 2020
c0fa5ad
Reverting default tab for invariant perspective UI
Jul 1, 2020
3952b23
Reverting UI changes related to 1e-6 factor
Jul 2, 2020
3a364c7
Changed default contrast to 8e-06 as discussed in PR 1590
Jul 2, 2020
5939b17
Updated warning in invariant_help about default contrast
Jul 2, 2020
5e0592c
Fixing total invariant value
Jul 3, 2020
7f7a523
Disentangle total Q* from Q* from data
butlerpd Jul 3, 2020
574dc97
Tweak to Invariant help in 5.x
butlerpd Jul 3, 2020
8a607a9
Make Qmin and Qmax read only in invariant panel.
butlerpd Jul 4, 2020
0d50b7a
Addressing review findings on PR #1590 (plus new problems found)
butlerpd Jul 4, 2020
01456e7
Move from __future__ import to top of file else can't start SasView
Jul 5, 2020
812dcde
Tweaks and cleanups in 5.x invariant_help
Jul 5, 2020
4240977
Fix problem with vol_fraction calc and tweak to front matter in
butlerpd Jul 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 29 additions & 14 deletions src/sas/qtgui/Perspectives/Invariant/InvariantDetails.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,14 @@ def __init__(self, parent):

# invariant total
self.qstar_total = None
self.qdata = None
self.qhigh = None
self.qlow = None
self._model = None

self.progress_low_qstar = 0.0
self.progress_high_qstar = 0.0
self.progress_qstar = 100.0
self.progress_data_qstar = 100.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename progress_* to percentage_*. The code is using a progress bar to represent a fixed percentage, not one that updates as the calculation progresses. I spent some time looking for the loop which updated the progress without success.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed it is .... not ideal. However I am going to ask to defer this to the next effort. Indeed part of the naming comes from the fact that the widget chosen for this IS in fact a progress bar which is not ideal either and part of @smk78 issue with why low q is always reporting 0% when 4.x was reporting a value of 0.6%. Also I am not yet familiar enough with the code base much less Qt to make such wholesale changes and testing for unintended consequences (e.g. did I really catch all the calls using it) to feel comfortable doing this before a release with zero time to look at it.

For now that bit is just copied/pasted and keeping exactly the same style as the entire rest of the invariant GUI code.


def setModel(self, model):
""" """
Expand All @@ -77,13 +78,29 @@ def showDialog(self):
# Pull out data from the model
self.qstar_total = float(self._model.item(WIDGETS.W_INVARIANT).text())

self.txtQData.setText(str(self.qstar_total))
self.txtQDataErr.setText(self._model.item(WIDGETS.W_INVARIANT_ERR).text())

# Reset progress counters
self.progress_low_qstar = 0.0
self.progress_high_qstar = 0.0
self.progress_qstar = 100.0
self.progress_data_qstar = 100.0

# Reset numerical values
self.txtQData.setText(None)
self.txtQDataErr.setText(None)
self.txtQLowQ.setText(None)
self.txtQLowQErr.setText(None)
self.txtQHighQ.setText(None)
self.txtQHighQErr.setText(None)


# Q* from data
self.qdata = float(self._model.item(WIDGETS.D_DATA_QSTAR).text())
rozyczko marked this conversation as resolved.
Show resolved Hide resolved

self.txtQData.setText(str(self.qdata))
self.txtQDataErr.setText(self._model.item(WIDGETS.D_DATA_QSTAR_ERR).text())
try:
self.progress_data_qstar = (self.qdata/self.qstar_total)*100.0
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid bare except statements. In addition to the coding exceptions, they also block SystemExit and KeyboardInterrupt.

except Exception: is discouraged by lint as being unspecific. In this case it is just being used to protect against qstar_total == 0.0, which is better done with an if statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again am going to ask to defer this. This would be a change to the original structure of the code. The change here is another copy paste from the other identical sections below it. I would rather keep the style the same until we change all of them. Otherwise it becomes hard to follow (at least for me who is not a python ninja 😃 when each related bit is written with a different style.

self.progress_data_qstar = 'error'

# Low-Q
if self._model.item(WIDGETS.W_ENABLE_LOWQ).text() == "true":
Expand All @@ -107,11 +124,6 @@ def showDialog(self):
except:
self.progress_high_qstar = 'error'

try:
self.progress_qstar -= self.progress_low_qstar + self.progress_high_qstar
except:
self.progress_qstar = 'error'

# check values and display warning
if self.checkValues():
self.lblWarning.setText(self.checkValues())
Expand All @@ -127,10 +139,10 @@ def showDialog(self):
else:
self.progressBarHighQ.setValue(self.progress_high_qstar)

if self.progress_qstar == 'error':
if self.progress_data_qstar == 'error':
self.progressBarData.setValue(0)
else:
self.progressBarData.setValue(self.progress_qstar)
self.progressBarData.setValue(self.progress_data_qstar)

self.show()

Expand All @@ -149,10 +161,10 @@ def checkValues(self):
return warning_msg

msg = ''
if self.progress_qstar == 'error':
if self.progress_data_qstar == 'error':
msg += 'Error occurred when computing invariant from data.\n '
try:
if float(self.progress_qstar) > 100:
if float(self.progress_data_qstar) > 100:
msg += "Invariant Q contribution is greater than 100% .\n"
except ValueError:
# Text message, skip msg update
Expand Down Expand Up @@ -202,4 +214,7 @@ def checkValues(self):
msg += "The sum of all extrapolated contributions is higher " \
"than 5% of the invariant.\n"

if msg == '':
Copy link

Choose a reason for hiding this comment

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

Do we want to output it or it was set for debugging purposes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to output it or it was set for debugging purposes?

No. That can stay in. Similar messaging is in 4.x.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this was very deliberate (and took me a while to figure out). Currently, without this, the default is message in the details warnings box is: No Details on calculations available. Which is misleading since the implication to me (and confused me when I was using it at first) is that something went wrong.

NOTE: this is set in the panel initialization as the default entry at which point it is true that no details are avalialbe since the code running the calculation has not run so decided that changing it there was not correct?

Also, oddly line 59 sets the same message but is never used anywhere that I can see! (and changing it has no effect I could see). Was going to delete but decided I didn't know enough? Should we delete?

        self.warning_msg = "No Details on calculations available...\n"

msg = "No Warnings to report\n"

return msg
24 changes: 14 additions & 10 deletions src/sas/qtgui/Perspectives/Invariant/InvariantPerspective.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
# global
import sys
import os
import logging
import copy
import webbrowser
import numpy as np

from PyQt5 import QtCore
from PyQt5 import QtGui, QtWidgets
Expand All @@ -16,8 +14,6 @@
from sas.qtgui.Plotting.PlotterData import Data1D
import sas.qtgui.Utilities.GuiUtils as GuiUtils

# import sas.qtgui.Plotting.PlotHelper as PlotHelper

# local
from .UI.TabbedInvariantUI import Ui_tabbedInvariantUI
from .InvariantDetails import DetailsDialog
Expand Down Expand Up @@ -53,7 +49,7 @@ def __init__(self, parent=None):
# initial input params
self._background = 0.0
self._scale = 1.0
self._contrast = 1.0
self._contrast = 8.0e-6
self._porod = None

self.parent = parent
Expand Down Expand Up @@ -222,7 +218,6 @@ def plotResult(self, model):
# Set the button back to available
self.cmdCalculate.setEnabled(True)
self.cmdCalculate.setText("Calculate")
self.cmdStatus.setEnabled(True)

self.model = model
self.mapper.toFirst()
Expand Down Expand Up @@ -256,6 +251,8 @@ def calculateThread(self, extrapolation):
self.updateFromModel()
msg = ''

qstar_data = 0.0
qstar_data_err = 0.0
qstar_low = 0.0
qstar_low_err = 0.0
qstar_high = 0.0
Expand Down Expand Up @@ -292,7 +289,7 @@ def calculateThread(self, extrapolation):
calculation_failed = False

try:
qstar_total, qstar_total_error = inv.get_qstar_with_error()
qstar_data, qstar_data_err = inv.get_qstar_with_error()
except Exception as ex:
msg += str(ex)
calculation_failed = True
Expand All @@ -301,11 +298,9 @@ def calculateThread(self, extrapolation):
self.model.setItem(WIDGETS.W_INVARIANT, item)
item = QtGui.QStandardItem("ERROR")
self.model.setItem(WIDGETS.W_INVARIANT_ERR, item)

try:
volume_fraction, volume_fraction_error = \
inv.get_volume_fraction_with_error(self._contrast)

except Exception as ex:
calculation_failed = True
msg += str(ex)
Expand All @@ -331,8 +326,10 @@ def calculateThread(self, extrapolation):
surface = None

if (calculation_failed):
self.cmdStatus.setEnabled(False)
logging.warning('Calculation failed: {}'.format(msg))
return self.model
self.cmdStatus.setEnabled(True)

low_calculation_pass = True
high_calculation_pass = True
Expand Down Expand Up @@ -416,8 +413,15 @@ def calculateThread(self, extrapolation):
reactor.callFromThread(self.updateModelFromThread, WIDGETS.W_SPECIFIC_SURFACE_ERR,
surface_error)

qstar_total = qstar_data + qstar_low + qstar_high
qstar_total_error = np.sqrt(
qstar_data_err * qstar_data_err
+ qstar_low_err * qstar_low_err + qstar_high_err * qstar_high_err)

reactor.callFromThread(self.updateModelFromThread, WIDGETS.W_INVARIANT, qstar_total)
reactor.callFromThread(self.updateModelFromThread, WIDGETS.W_INVARIANT_ERR, qstar_total_error)
reactor.callFromThread(self.updateModelFromThread, WIDGETS.D_DATA_QSTAR, qstar_data)
reactor.callFromThread(self.updateModelFromThread, WIDGETS.D_DATA_QSTAR_ERR, qstar_data_err)
reactor.callFromThread(self.updateModelFromThread, WIDGETS.D_LOW_QSTAR, qstar_low)
reactor.callFromThread(self.updateModelFromThread, WIDGETS.D_LOW_QSTAR_ERR, qstar_low_err)
reactor.callFromThread(self.updateModelFromThread, WIDGETS.D_HIGH_QSTAR, qstar_high)
Expand Down
4 changes: 2 additions & 2 deletions src/sas/qtgui/Perspectives/Invariant/InvariantUtils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
'W_INVARIANT',
'W_INVARIANT_ERR',
# for the details widget
'D_TOTAL_QSTAR',
'D_TOTAL_QSTAR_ERR',
'D_DATA_QSTAR',
'D_DATA_QSTAR_ERR',
'D_LOW_QSTAR',
'D_LOW_QSTAR_ERR',
'D_HIGH_QSTAR',
Expand Down
18 changes: 18 additions & 0 deletions src/sas/qtgui/Perspectives/Invariant/UI/InvariantDetailsUI.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@
<property name="toolTip">
<string>Extrapolated invariant from low-Q range.</string>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
Copy link
Contributor

Choose a reason for hiding this comment

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

On Mac there is no visual indication that a field is read-only. You click and either a cursor does or does not appear. When it doesn't you click a bunch of times to make sure you didn't miss the entry field when clicking.

Maybe set the background for read-only fields to the same color as the form so they look like the labels that they are?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly that is true for PC also. This is the quick improvement over all those fields being editable making the user think they were changing something when in fact the code was ignoring them.

I can also disable the box which will make it gray like the background but then the text can no longer be copied to the clipboard and more annoyingly it is grey as well instead of black which I personally find more annoying that the current. As I stated somewhere (maybe in the commit message? if so that would be pointless since nobody reads those 😃 I do not know yet how to achieve what we had in 4.x. Moreover I think this is a general thing in the ESS_GUI and standardizing output boxes would be I think the right answer - but that requires a discussion I think.

</widget>
</item>
<item row="0" column="2">
Expand All @@ -103,6 +106,9 @@
<property name="toolTip">
<string>Uncertainty on the invariant from low-Q range.</string>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item row="0" column="4">
Expand All @@ -124,6 +130,9 @@
<property name="toolTip">
<string>Invariant in the data set's Q range.</string>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item row="1" column="2">
Expand All @@ -138,6 +147,9 @@
<property name="toolTip">
<string>Uncertainty on the invariant from data's range.</string>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item row="1" column="4">
Expand All @@ -159,6 +171,9 @@
<property name="toolTip">
<string>Extrapolated invariant from high-Q range.</string>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item row="2" column="2">
Expand All @@ -173,6 +188,9 @@
<property name="toolTip">
<string>Uncertainty on the invariant from high-Q range.</string>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item row="2" column="4">
Expand Down
10 changes: 8 additions & 2 deletions src/sas/qtgui/Perspectives/Invariant/UI/TabbedInvariantUI.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>422</width>
<height>473</height>
<width>544</width>
<height>489</height>
</rect>
</property>
<property name="sizePolicy">
Expand Down Expand Up @@ -143,6 +143,9 @@
<property name="enabled">
<bool>true</bool>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item>
Expand All @@ -157,6 +160,9 @@
<property name="enabled">
<bool>true</bool>
</property>
<property name="readOnly">
<bool>true</bool>
</property>
</widget>
</item>
<item>
Expand Down
2 changes: 1 addition & 1 deletion src/sas/qtgui/Perspectives/Invariant/UnitTesting/InvariantPerspectiveTest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def testDefaults(self):
self.assertEqual(self.widget.txtTotalQMax.text(), '0.0')
self.assertEqual(self.widget.txtBackgd.text(), '0.0')
self.assertEqual(self.widget.txtScale.text(), '1.0')
self.assertEqual(self.widget.txtContrast.text(), '1.0')
self.assertEqual(self.widget.txtContrast.text(), '8e-06')
self.assertEqual(self.widget.txtExtrapolQMin.text(), '1e-05')
self.assertEqual(self.widget.txtExtrapolQMax.text(), '10')
self.assertEqual(self.widget.txtPowerLowQ.text(), '4')
Expand Down
Binary file modified src/sas/qtgui/Perspectives/Invariant/media/image_invariant_details.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified src/sas/qtgui/Perspectives/Invariant/media/image_invariant_load_data.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified src/sas/qtgui/Perspectives/Invariant/media/image_invariant_option_tab.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.