Invariant Refactor#3789
Conversation
5b6a461 to
c3455e7
Compare
83c64de to
ca161cb
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Invariant perspective UI with a new tabbed interface that separates input/output (tab 1) from extrapolation options (tab 2). It adds SLD contrast calculation from volume fraction as a corollary to the existing volume fraction calculation from contrast, and replaces the old extrapolation slider with the Corfunc slider.
Key Changes
- New two-tab UI structure with improved organization of invariant calculations and extrapolation parameters
- Addition of
get_contrast()andget_contrast_with_error()methods to calculate SLD contrast from volume fraction - Integration of CorfuncSlider for extrapolation visualization with Q-value inputs replacing number-of-points inputs
- Progress bars added to extrapolation tab to show Q* contributions from low-Q, data, and high-Q regions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| invariant.py | Added two new methods for contrast calculation from volume fraction; fixed typo in comment ('constrast' → 'contrast') |
| TabbedInvariantUI.ui | Complete UI restructure with new tab layout, volume fraction/contrast toggle, and extrapolation controls |
| InvariantUtils.py | Added new widget constants for extrapolation tab and volume fraction fields; added safe_float() helper function |
| InvariantPerspective.py | Major refactor with new extrapolation logic, progress bar updates, volume fraction auto-calculation, and slider integration |
Comments suppressed due to low confidence (1)
src/sas/qtgui/Perspectives/Invariant/InvariantUtils.py:55
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DrPaulSharp
left a comment
There was a problem hiding this comment.
Looking good so far! Alongside the code comments here, I have a number of suggestions from my functionality tests:
- The text boxes for the q ranges look editable, but they (correctly) are not. A better visual cue to this would be helpful.
- The type used for units appears bold for all except the cm-1 used for the background. All units should look like this if possible.
- When entering "2e-6" for the contrast, it turns red halfway through. You probably want to change to an
editingFinishedsignal here. - For the output boxes, I think it would make sense to grey out the contrast box when contrast is the input, and similarly for the volume fraction (i.e, mirroring how the input boxes look)
- The second tab is called "Page", I'd suggest "Extrapolation" makes more sense.
- In the extrapolation tab, why do we have the "name" text box? Would it make more sense to have the name of the dataset from the first tab?
- The text boxes for the Extrapolation limits are too narrow.
- The radio buttons for the low-q extrapolation are a bit squashed.
- The extrapolation box goes wider to the left than the Invariant chart box.
- !! Given the above points, I'd suggest making the perspective window a bit wider. It's narrower than Corfunc, so we don't need to worry too much about space. !!
- The high q extrapolation is a power law rather than Porod - note that Porod only applies when the power is equal to 4. See https://www.sasview.org/docs/user/qtgui/Perspectives/Invariant/invariant_help.html, https://www.sasview.org/docs/user/qtgui/Perspectives/Corfunc/corfunc-technical.html and lines 256-265 of
sascalc/invariant/invariant.py. - The low q extrapolation can be either power law or Guinier, but the text box and slider always say Guinier.
- !! Again, given the above, I think switching the text labelling the text boxes and on the slider to "low-q" and "high-q" makes more sense.
- There is always a power displayed for the low q extrapolation, even when Guinier is used. Can we extract and display the Guinier parameters, as we do in Corfunc?
- Note that some comments in
sascalc/invariant/invariant.pyincorrectly state that Guinier puts the radius of gyration to a power of 2/3 - this should be corrected. - In the extrapolation tab, the "A-1" at the end of the ranges text boxes should have the -1 as a superscript.
- We need to think about the format of Guinier for this perspective compared to Corfunc (note the docs use equaivalent, but different parameters). This is one to discuss more widely.
- For the output, I think it makes sense to have both volume fractions to make things completely clear.
- If one does a specific surface calculation, but then removes the Porod constant, the box turns red and the value from the last calculation is maintained. Instead, the box should stay white and the value cleared when the calculation button is pressed.
- I would move the radio buttons for contrast and volume fraction to the right hand side, so they are clearly grouped with the text boxes.
- The error for a surface calculation is always None.
- You've already noted this, but simply running a calculation reduces the two datasets plotted to one.
- If a volume fraction outside 0-1 is entered the calculation cannot be performed and the boxes turn red (correctly). I think a message of some kind explaining why would be useful. What do you think?
- When I delete the data, I get an error: "Traceback (most recent call last): File "C:\Users\gnn85523\projects\sasview\src\sas\qtgui\MainWindow\GuiManager.py", line 1404, in showPlot self.filesWidget.displayData(plot) File "C:\Users\gnn85523\projects\sasview\src\sas\qtgui\MainWindow\DataExplorer.py", line 1156, in displayData main_data = GuiUtils.dataFromItem(plot_item.parent()) ^^^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'parent'"
Good luck with these, let me know if you need a hand.
| from sas.qtgui.Perspectives.Corfunc.CorfuncSlider import CorfuncSlider | ||
| from sas.qtgui.Plotting import PlotterData | ||
| from sas.qtgui.Plotting.PlotterData import Data1D, DataRole | ||
| from sas.sascalc.corfunc.calculation_data import ExtrapolationInteractionState, ExtrapolationParameters |
There was a problem hiding this comment.
We should look to extract these common routines from the Corfunc perspective so that we do not introduce cross-perspective dependencies.
Hey Paul! Thanks for your eagle-eyed review! I have added commits addressing most of them.
Volume fraction 2 gets removed in the next update with uncertainties, as it is redundant. Therefore, I think this can be left out for now.
Currently, uncertainties in input are not supported, hence the uncertainty in specific surface is just zero, which was commented out in
I am putting this on hold for now for wider discussion. |
DrPaulSharp
left a comment
There was a problem hiding this comment.
This is looking very good, I have a few more comments.
- The name of the dataset in the extrapolation tab should be greyed out (like in the Invariant tab)
- Line 182 of
invariant.pyneeds to be corrected to:Compute $F(x) = s * \exp\left(-(r x)^{2}/3\right)$. - For the Porod constant, the box no longer turns red when a porod constant is removed, but if it is removed and another calculation run, the previous Porod constant is used. Instead, if the value is removed, a specific surface calculation should not be run.
As discussed there are some other issues which we can move to a future PR. We should also wait for feedback from @butlerpd
|
Thanks for pointing out that issue @DrPaulSharp. I have added a comment to that issue |
95be4c0 to
a792f68
Compare
The sliders are added in the |
We want to keep the sliders on the plot as a visual guide, so they are needed for that, but as per previous discussions, we want to disable them from updating the extrapolation parameters. |
UI Corfunc label Label correction sigfig
6ec15c8 to
911c689
Compare
DrPaulSharp
left a comment
There was a problem hiding this comment.
Looks fantastic, thanks for your hard work on this and pushing to make it as good as possible.
There's just one more thing from me, could you please switch the colours of the lines on the extrapolation plot - currently the high-q line is orange and the low-q line is green, which is the opposite to the colours used on the slider.
Description
Note: This branch is based on Andrew Jackson's (@ajj) branch, which started refactoring the perspective to incorporate volume fraction as an input.
Changes the UI for Invariant perspective:
The first tab contains all the input/output for the invariant/contrast/volume fraction calculation
The second tab contains the extrapolation options and parameters
The extrapolation slider has been replaced with the slider from Corfunc Perspective (the gradient bar in the extrapolation tab), and the slicers on the plot are just a visual guide
Progress bars from the status window are also visible on the extrapolation tab
This also replaces the 'number of points' input with a Q value input
The data plot is now shown upon loading data for easier visualisation
Adds SLD contrast calculation from volume fraction
Users can now choose between the two corollary calculations
Known issues:
The plot displayed after loading the data shows the data points twice (overlapping). This had to be done due to how the plotting works, and has been left unresolved in anticipation of the plotting refactorResolved: the initial plot now uses the functionality of the
Create Newbutton at the bottom of Data Explorer. Extrapolated plots still use the invariant plotting, replacing the initial plot.Fixes #1496
How Has This Been Tested?
Manually ran the software with different datasets to check the functionality, and compared with the old version to reproduce the same results.
Review Checklist:
Documentation (check at least one)
Installers
Licensing (untick if necessary)