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 1 commit
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
34 changes: 20 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,20 @@ 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

# 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 +115,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 +130,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 +152,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 +205,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
10 changes: 7 additions & 3 deletions src/sas/qtgui/Perspectives/Invariant/InvariantPerspective.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,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 @@ -287,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 Down Expand Up @@ -412,13 +414,15 @@ def calculateThread(self, extrapolation):
reactor.callFromThread(self.updateModelFromThread, WIDGETS.W_SPECIFIC_SURFACE_ERR,
surface_error)

qstar_total += qstar_low + qstar_high
qstar_total = qstar_data + qstar_low + qstar_high
qstar_total_error = np.sqrt(
qstar_total_error * qstar_total_error
qstar_data_err * qstar_total_error
Copy link

Choose a reason for hiding this comment

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

Shouldn't be "qstar_data_err * qstar_data_err" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Will need @butlerpd to check this one. Where these things are calculated and passed from is a bit of a can of worms as you no doubt discovered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @smk78 for letting me take a look - sometimes weird things happen in this invariant code indeed.... but in this case I am thoroughly embarrassed. @wpotrzebowski is correct. Here we are just adding the errors in quadrature not doine sophisticated error propagation through a function (where something like this might be used). My bad (must have been in a hurry to get to something else ☹️ really good catch on the review!

+ 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