Slicer extension 1344

[butlerpd]

This PR attempts to allow the box slicers to take slices whose center is not 0,0. if using the full q range of the long dimension this would be the same as giving slices at constant qx yielding I(qy|qx=constant) for example.

[butlerpd]

ready for testing on Win

[butlerpd]

It now mostly works!! Remaining things to address that I found through testing on a (slow) Windows 10 machine.

• We need to add the Qcenter (e.g. [Qx_center, Qy_center]) to the slicer parameter editor.
• Now that Qcenter is not alwayas [0,0], we need to change the xmax and ymax to no longer be relative to [0,0]. I can think of two possibilities. I think the second is more useful for the typical user:
  • make them relative to Qcenter
  • change the to be a delta Q. In other words the width of the slab in Q
• Minor issue that may not be possible to fix easily and could easily be punted to a future ticket is that depending on the data set the Qcenter handle is not obvious till you move the cursor over it.
• The Q center handle is not cleaned up when a new slicer is called. If one brings up the x slicer and moves the center around then decides to switch to the y slicer (or any other slicer for that matter), the center handle at the time of the change seems to remain permanently fixed on the plot. It cannot be removed by toggling log/lin or changing the color scale even. The result can be quite interesting if switching back and forth between x and y slicers: It starts to look like the plot has chickenpox 😄
• There are two, perhaps related perhaps not, issues with the graphical motion of the center handle
  • when using the cursor to move the box center, the center moves to the last captured cursor position BEFORE the current position. Thus when letting up on the mouse (drag) button, the center position will jump to the final cursor position. This makes precise positioning very difficult.
  • While not 100% reproducible it seems to be 80% reproducible that if moving the Qcenter in the negative part of the plot (negative Qx for the x slab or negative Qy for the y slab), the GUI seems to miss the unclick action of the mouse if the cursor is still moving before the center handle can "jump" to its final location. Stopping the motion and left clicking while the cursor is motionless seems to resolve the issue though every once in a while it requires several clicks to let go.
• Finally, unit tests probably need to be updated and/or added for the new functionality?

[rozyczko]

I get the following two errors when testing the functionality:

  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Binder.py", line 291, in _onMotion
    self.trigger(self._hasclick, 'drag', event)
  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Binder.py", line 217, in trigger
    processed = self._actions[action][artist](ev)
  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Slicers\BaseInteractor.py", line 142, in onDrag
    self.restore(ev)
TypeError: restore() takes 1 positional argument but 2 were given

File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Slicers\BaseInteractor.py", line 129, in onRelease
    self.moveend(ev)
  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Slicers\BoxSlicer.py", line 538, in moveend
    self.base.moveend(ev)
  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Slicers\BoxSlicer.py", line 248, in moveend
    self._post_data()
  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Slicers\BoxSlicer.py", line 747, in _post_data
    self.post_data(SlabX, direction="X")
  File "C:\Users\piotr\projects\sasview\src\sas\qtgui\Plotting\Slicers\BoxSlicer.py", line 198, in post_data
    boxavg = box(self.data)
  File "C:\Users\piotr\projects\sasview\src\sas\sascalc\dataloader\manipulations.py", line 460, in __call__
    return self._avg(data2D, 'x')
  File "C:\Users\piotr\projects\sasview\src\sas\sascalc\dataloader\manipulations.py", line 430, in _avg
    raise ValueError(msg)
ValueError: Average Error: No points inside ROI to average...

[butlerpd]

Thanks @rozyczko I just noticed the second error on exiting the application but failed to see the first. Looking more closely I too get that error. This could easily solve a couple of the GUI problems I'm guessing?

[rozyczko]

Code looks very good, there are some very minor stylistic issues left.
If the errors seen in my local build test are reproducible, then those will have to be addressed.

[kmothander]

I've tested this locally on Linux.

The errors that occurred previously are fixed, sometimes I get the error "ValueError: Average Error: No points inside ROI to average...", but not 100% reproducible, and only happens at -Qx. I'm not really sure where this error comes from or how to fix it...

I've also fixed so that the box is placed in the middle of the plot not just at Qx = Qy = 0. However, if the plot only contains data at negative Qx (and negative Qy) then I get the same error as above.

I've added a x_center and y_center to the parameter list. The parameters can be edited through the Edit slicer parameter window.

Question:
As @butlerpd mentioned, should there be other parameters then x_max, y_max, x_center and y_center? I think it should be possible to have x/y_center and then an sizes of the box, in the same way as for the BoxSum. Or should the size and placement of the box be defined in some other way?

[butlerpd]

LOL .. I see that it looks like these commits break all the unit tests .. which makes sense since the behavior has been altered. Those will need to be adjust clearly to reflect the new correct behavior.

Regarding the question about how to present the width variables that is a question of what might be easier/more intuitive for a user. My thinking was that for the purpose at hand my guess is that the user will more likely be thinking in terms of delta Q around the center rather than qmax?

Another usability question that has come to mind is whether for the boxslicer as opposed to boxsum, it would not make much more sense (and be less confusing to a new user) if the default box was not drawn as a square from -.5 imagedQ to +.5 imagedQ in both directions. I would think something like -3/4 imagedQ to + 3/4 imagedQ (or even -1 to 1) in the direction of the q being scanned and maybe -.2 to +.2 in the direction of q being integrated.

Note that by imagedQ I mean qmax-qcenter of the displayed data (image)

[kmothander]

@butlerpd
I have no idea how to change the unit test...

To your other questions I can change and add the new functionality. Changing the parameter list and see if I can change the initial size of the box!

[wpotrzebowski]

@butlerpd will look at it

[butlerpd]

See if @butlerpd can address at June hackathon otherwise remove PR and wait till someone has time

[butlerpd]

There is a difference between very limited resources on a complex project and a stale project :-)

[lucas-wilkins]

This branch is 9 commits ahead, 1374 commits behind main.

[lucas-wilkins]

There is a difference between very limited resources on a complex project and a stale project :-)

Unlike issues, branches get behind main and merging them gets harder and harder. If you don't want a stale label, please keep it up to date with main, preferably with log10(commit_difference) < 2.

[dehoni]

Side note - There are changes to sas_gen.py and associated unit tests in here. Is this a merge-related issue? I think @dehoni should take a look to be sure we don't remove necessary functionality.

Indeed these were spurious commits that came in from a couple of poorly executed merging/reverting/munging 2 years ago. This was fixed by my last commit cd3e7fd (see commit notes for more details). The files mentioned are no longer part of the changes in this PR

These files are part of main. It must have been part of an update of the branch. Anyway, the files are unrelated to this PR feature and only work on the general scattering calculator.

[butlerpd]

All comments and concerns should now have been addressed. Also in testing a few new bugs were discovered and fixed and all user inputs now checked to avoid failures as documented in each of the commit messages.

One question: The tests now log warning when the user tries to provide an invalid ROI. However, Warnings do not trigger opening the GUI console. Only Error level logging does. However the warning is meant as an aid to the user. Can we have warnings bring up the console? or should these messages be labelled as error even though I would argue they technically do not rise to the error level at this point?

[krzywon]

This is looking much better. I think almost there.

[krzywon]

Minor nitpick at this point: The graphical box interactors won't allow me to extend/move to the edge of the data anymore, but that is mostly a visual issue.

[krzywon]

I just tested the latest changes (from code, not the installer) and the user experience is much better. The box sides are coerced to the data limits and no warnings.

Assuming the installers work, I think this can be merged.

[butlerpd]

It still needs to have the ci.yml reverted. before merging. Will do after my meeting.

We probably should also say something in the docs. The coercion to the edge worked a bit better for me the old way but the big advantage here that I think trumps all minor differences in usage is the fact that now you can slide the initial box up and down (or sideways depending on which slicer it is) without shrinking the box.

Note I plan to update the docs in another PR. A lot of the changes will need documentation updates as well.