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
Ess gui 1574 invariant #1590
Ess gui 1574 invariant #1590
Conversation
Replacing the unit test data, updated the unit test files by copying the 4.x files as the two master copies are identical. Also updates the default contrast value in the GUI
updated the sascalc file now. Again looks like it can be a direct copy as there seems to have been no changes between master and ESS_GUI. this should fix the actual factor of 2 as well as the totally bogus uncertainties. Left todo would be the help.rst and a couple of GUI fixes.
Added (I think)the value of "NONE"instead of 0 to the gui reporting of Sv uncertaining; fixed a doc string error htat needs to be backported to 4.x: updated the documentation file including some notes in the "how to use" section which may need to be backported to 4.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment about the comparison with floats. The sascalc stuff is assumed to be correct, having been copied from 4.x
@@ -318,6 +318,9 @@ def calculateThread(self, extrapolation): | |||
try: | |||
surface, surface_error = \ | |||
inv.get_surface_with_error(self._contrast, self._porod) | |||
if surface_error == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surface_error
is a float. Comparing floats is a tricky thing, maybe we should do the comparison against some very small value?
if abs(surface_error) < 0.00001
or something like if math.isclose(surface_error, 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test against zero happens to not a problem in this case because the get_surface_with_error
function always returns zero for the surface error. If it were actually calculated, you probably want to use surface_error < 1e-7*surface
or some such rather than an absolute test so you don't have to worry about the units used for the surface calculation.
There are bigger problems, though. The setting of the surface_error
field is overridden on line 417, at least for the case when bool(surface) is True
. That means that when surface is non-zero, then a zero will be entered into the surface error field overriding the setting "NONE" being set here.
That if surface:
test also means that when the get_surface_with_error
function returns zero the value in the surface field will not be updated, even if it is currently not zero.
A further problem is if an exception is raised here then surface
is undefined, and the if surface:
test will throw an exception.
Still another question/concern is that this code is calling setItem
directly, whereas line 417 is using reactor.callFromThread
to call setitem
— why the difference?
So the conditions you want seems to be:
if surface present:
if get_surface_with_error throws an exception:
display error in surface and surface error fields
else:
display surface value in surface field
if surface error is zero:
display none in surface error field
else:
display surface error value in surface error field
else:
display none in surface and surface error fields
Maybe hijack updateModelFromThread
with:
if value is None:
item = QtGui.QStandardItem("NONE")
elif np.isnan(value):
item = QtGui.QStandardItem("ERROR")
else:
item = QtGui.QStandardItem(str(float('%.3g' % value)))
self.model.setItem(widget, item)
Then your logic here would be:
if self._porod:
try:
surface, surface_error = ...
if surface_error < 1e-7*surface:
surface_error = None
except Exception as ex:
calculation_failed = True
msg += str(ex)
surface = surface_error = np.NaN
else:
surface = surface_error = None
and below call updateModelFromThread
for surface and surface_error without the if surface:
test.
You can do something similar for the qstar logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re "... the if surface: test will throw an exception.": This is not an issue because line 335 checks for calculation_failed
.
You still may want to use reactor.callFromThread
to update the fields within the exception code, or better yet, use calculation_failed
condition to set the extrapolation values to None
so that the form update logic is done together as a block at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks guys. This was in fact the one bit of code I was hoping to get feedback on as there was no copy paste from 4.x possible here and while we dealt with in wx, this code is beyond my monkey see monkey do approach (having not seen before 😃 so what we are trying to do is this: currently the calculation was disabled because it was totally bogus and moreover there is no information with which to compute the uncertainty. That should be another ticket (I need to check). The ideas was that if not calculated the GUI should show "none" whereas if it is calculated (at some point in the future when the GUI allows the necessary input AND the user chooses to provide it), no matter how small it is, it should show the numeric value.
Is what you suggest above .. which I've only partially understood so far I'll admit (not had time to really think about it yet) or am I totally barking up the wrong tree here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you assign zero to a value you can reliably test whether it is zero, but you won't be able to distinguish that from something that is accidentally zero because of floating point math.
If it is possible that the calculated value is zero and you want to display 0.0, then you should return something else (None?) from the sascalc function when the value is not calculated.
Re reactor.callFromThread
: I do not know if it is required. In wx you need to use the same thread for all GUI operations or weird things can happen, but I don't know what happens in qt. Maybe @rozyczko can answer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if the return value has to be a float it cannot be "None" right? maybe nan? but that suggest and error. Or are you allowed to return a "None" object instead of a float? I guess I can try..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can return anything you want . All Python values are objects that carry their type along with them, even floats, and python does nothing to enforce the type of the argument received or the returned value. The caller has to sort it out by using code like if value is None:
.
We can hint what we think the type should be, which is handy for the maintainer and can let automated tools (e.g., mypy) check that you are using types correctly. In that case we sould define the function as follows:
from typing import Optional, Tuple
def get_surface_with_error(
self,
contrast: float,
porod_const: float,
extrapolation: Optional[str]=None,
) -> Tuple[float, Optional[float]]:
...
or in python2.7 style as used by sasmodels:
def get_surface_with_error(self, contrast, porod_const, extrapolation):
# type: (float, float, Optional[str]) -> Tuple[float, Optional[float]]
...
Add the 3 new test data sets used in the Invariant unit tests. They contain extra information and should be good demo type data as well.
The code in the PR was incorret as pointed out by P. Kienzle, since the value in the uncertainty box is set from a call at the end of the routine. Only if the call to surface fails would setting the value work. Instead we leave InvariantPerspective as originally written and make the changes in invarian.py. ds now returns None if it is not computed and a value if it is. InvariantPerspective seems to handle it ok.
As discussed, removing this note till we can define a consistant way to flag errors in prior code for maximum impact.
5.0 ready for testing on Win |
In testing these changes to address the review a number of other issues have arisen that cannot be addressed here. These will be documented in separate tickets. |
5.0 ready for testing on OSX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linting and styling comments.
else: | ||
ds = math.sqrt(_dporod_const**2 + 2 * (_porod_const | ||
* _dcontrast / contrast)**2) / ( | ||
2 * math.pi * contrast**2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be testing for var is None
rather than var == None
. These will usually give the same result but lint complains if you use the ==
test. "Usually" because var == None
calls var.__eq__(None)
which can do whatever it wants with a None
value, including returning True
instead of False
. The is
test is also faster since it just needs to check if the object ids match (their location in memory), and does not need to call var.__eq__
.
That said, since it is always None
for the moment, you don't need any of this code, and can simplify it to:
s = self.get_surface(contrast=contrast, porod_const=porod_const)
# Until contrast and are provided the return nothing.
ds = None
# When they are available, use the following:
# if dporod_const is None:
# dporod_const = 0.
#if dcontrast is None:
# dcontrast = 0.
#ds = math.sqrt(dporod_const**2 + 2 * (porod_const * dcontrast / constrast**2)
# / (2 * math,.pi * contrast**2) * 1e-8
return s, ds
|
||
ds/s = sqrt[(dcp/cp)**2 + 2* (dcontrast/contrast)**2] | ||
|
||
which gives (this should be checked before using in anger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent to 4 not 8.
""" | ||
Invariant with low-Q extrapolation | ||
Tes the Invariant with the low-Q Guinier extrapolation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
self.assertEqual(qstar1, qstar) | ||
self.assertAlmostEqual(qstar, 9.458239e-5,delta=1e-6) | ||
self.assertAlmostEqual(v, 0.01000, delta=1e-4) | ||
self.assertAlmostEqual(s , 6.000e-5, 7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not checking ds
, dv
return values. Don't need it for all of them, but putting it in for one of them will remind whoever implements the uncertainty calculation that ds, dv need to be tested as well.
self.assertAlmostEqual(dqs_extr, math.sqrt(dqstar*dqstar + delta_dqs_extr*delta_dqs_extr), 10) | ||
|
||
self.assertAlmostEqual(dqs_extr, math.sqrt(dqstar*dqstar \ | ||
+ delta_dqs_extr*delta_dqs_extr), 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8 discourages the use of \
as a line extender (python gets confused sometimes if there is a space afterward, or if there is some problem with CRLF endings). You do not need it within parentheses.
Better would be to break at a higher level semantic boundary:
self.assertAlmostEqual(
dqs_extr, math.sqrt(dqstar*dqstar + delta_dqs_extr*delta_dqs_extr), 10)
or
self.assertAlmostEqual(
dqs_extr,
math.sqrt(dqstar*dqstar + delta_dqs_extr*delta_dqs_extr),
10)
This appears in a number of places, not all of them noted in this review.
@@ -473,10 +540,11 @@ def setUp(self): | |||
self.scale = 1.5 | |||
self.rg = 30.0 | |||
x = np.arange(0.0001, 0.1, 0.0001) | |||
y = np.asarray([self.scale * math.exp( -(q*self.rg)**2 / 3.0 ) for q in x]) | |||
y = np.asarray([self.scale * | |||
math.exp( -(q*self.rg)**2 / 3.0 ) for q in x]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break at higher level semantic boundaries. Also, lint will complain about space after open or before close parentheses:
y = np.asarray([self.scale * math.exp(-(q*self.rg)**2 / 3.0)
for q in x])
@@ -521,89 +598,103 @@ def setUp(self): | |||
self.scale = 1.5 | |||
self.rg = 30.0 | |||
x = np.arange(0.0001, 0.1, 0.0001) | |||
y = np.asarray([self.scale * math.exp( -(q*self.rg)**2 / 3.0 ) for q in x]) | |||
y = np.asarray([self.scale * | |||
math.exp( -(q*self.rg)**2 / 3.0 ) for q in x]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
@@ -348,10 +397,11 @@ def setUp(self): | |||
self.scale = 1.5 | |||
self.rg = 30.0 | |||
x = np.arange(0.0001, 0.1, 0.0001) | |||
y = np.asarray([self.scale * math.exp( -(q*self.rg)**2 / 3.0 ) for q in x]) | |||
y = np.asarray([self.scale * | |||
math.exp( -(q*self.rg)**2 / 3.0 ) for q in x]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as below
5.0 ready for testing on OSX |
I've been testing PR on mac. When invariant cannot be calculated ("ERROR" appears), clicking on "Status" button generates |
5.0 ready for testing on OSX |
…asview into ESS_GUI_1574_Invariant
5.0 ready for testing on OSX |
5.0 ready for testing on Win |
5.0 ready for testing on Linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally code looks good to me. The only issue that I spotted is qstar_total_error assignment (please check comment to the code)
qstar_total_error = np.sqrt( | ||
qstar_total_error * qstar_total_error | ||
qstar_data_err * qstar_total_error |
There was a problem hiding this comment.
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" ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -202,4 +205,7 @@ def checkValues(self): | |||
msg += "The sum of all extrapolated contributions is higher " \ | |||
"than 5% of the invariant.\n" | |||
|
|||
if msg == '': |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stylistic issues
* addresses lint issues form Paul K. and Piotr. Addresses * addresses error in Q* total uncertainty (Wojciech) * Addresses detail output boxes being editable (now made readonly) *Redid ds computation in invariant. In the fixing for Paul's comment the fact that unit conversions were required was lost. Also on relooking at the equation dimensional analyis indicated I had gotten it wrong. Redid the derivation to get the correct equation (check with two different methods and dimensional analyis works) * Also apparently forgot to commit the partial fix for #1607
Ok ... I have addressed all comments now I think, including making all (I think) of the output only boxes that were editable now only read-only. I note that in 4.x we made the equivalent of "disabled" rather than read only so it was dead obvious that they were output only. There is the subtlety though that in Qt, disable also greys the font besides the background. I also apparently had failed to push the partial fix to #1607 which now clears all the detail values before a new calculation. It should be almost as simple to fix the other part which is the persistence of the extrapolation curves but I cannot see how to do it yet. I can see where they are calculated IF extrapolation is true - but they then persist forever until another extrapolation=true calc. What we need is to have them cleared every time a calculation is done so that if extrapolation=false they don't show up on the plot. Finally a bit moot but spent most of my time rederiving ds which was clearly still wrong -- that needs backporting to 4.x PR Otherwise I am good with this being merged unless y'all find something new. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
Having pulled the branch and built it, when I tried to run it I got this error:
I have resolved it by moving the from __future__ import from line 19 to line 9 |
Building the 5.x ticket1574-invariant branch: More Good news: The Bad news: |
Thanks @smk78.... but I just looked at the code to remind myself and in fact the GUI code does not choose which invariant to use in calculating the vol_fraction. It only sends the contrast. It does have to tell the computation which extrapolations to use (high, low or both) but I think that is being done correctly? Also my tests show that vol fraction is giving the correct result. can you tell me which data set you are using (or post here if it is not one in the test suite) along with the different values you are seeing? |
Just using the \test\1d_data\AOT_Microemulsion-Drop_Contrast.xml
Which works out as Invariant Total = 0.083 . A |
Sorry @smk78 ... I think I'd figured out on Friday that was your data set from the figure posted above. My memory is not so good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Mac the status dialog is not part of the MDI window and so it mysteriously vanishes when clicking back to the options dialog to change entries. Since it is still displayed, but simply hidden behind the main window, the Status button remains gray. Is it easy to add the dialog to the MDI, or perhaps raise the window to the front when the gray status button is clicked?
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -89,6 +89,9 @@ | |||
<property name="toolTip"> | |||
<string>Extrapolated invariant from low-Q range.</string> | |||
</property> | |||
<property name="readOnly"> | |||
<bool>true</bool> | |||
</property> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I think I see the problem @smk78 and you would be correct (my examples did not have much contribution from the extrapolations 😦 It seems that just setting the state of the extrapolation is not sufficient -- you must also call the volume fraction method not only with the contrast but also the extrapolation you want to use (low, high,or both) if no extrapolation string is passed the code assumes extrapolation = None 🤕. Will try to fix asap but am rapidly becoming a pumpkin as far as SasView is concerned I'm afraid. |
invariant.py Added extrapolation to the vol fraction call so that it returns the value based on the chosen extrapolation not just from Q*data. Also realized that the problem encountered by Steve of the import from future not being the first import was due to eclipse being helpful and adding an import pi which I did not even need. So reverted to the original (without the pi import and moving the from future import to after the doc string.
5.0 ready for testing on OSX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked numerical values and they seem to agree with 4.x. I think this is what we can achieve for 5.0.3.
Have pulled and re-built the 5.x ticket1574-invariant branch and re-compared it against the 4.x ticket1434-invariant-documentation branch, and... ...I am delighted to report that both branches are now returning the same numbers for the same parameters! The one thing that I do notice is that if you specify a Porod Constant, though both branches return a value for the Specific Surface (and the same value!), 4.x gives NONE for the uncertainty whilst 5.x leaves the uncertainty box blank. But I think @butlerpd was aware of this. However, it is not a feature that should delay merging this branch any longer. |
Finally, just to add that this is not the end of matters: as our Issues list will testify, both the Invariant (and Correlation Function) Perspectives need a lot more TLC! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on W10/x64. This is now good enough to merge.
This addresses issue #1574 and is basically a forward port of the changes worked out for 4.3.
From the user perspective the most important changes are:
Other fixes include