-
Notifications
You must be signed in to change notification settings - Fork 47
Generalize Corfunc Slider #3833
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
base: main
Are you sure you want to change the base?
Conversation
6ec15c8 to
911c689
Compare
85024f0 to
1b89558
Compare
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.
Some points on appearance to address here. Once you've sorted out the colours, have a look at the slider before data is sent to the perspective, does it look right at that point?
Also, I've found a bug, if the slider is invalid in the invariant perspective in such a way that the boxes go red but the dialog is not shown (e.g., low q end is greater than high q start), then it is still possible to run a calculation with the invalid input. See screenshot:

| self._max = params.data_q_max | ||
|
|
||
| self._data_max = params.data_q_max | ||
| self._max = params.ex_q_max if params.ex_q_max is not None else params.data_q_max |
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.
What happens if individual parameters other than self._max are set to None?
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 do not think that is a possibility. Only ex_q_min and ex_q_max can be None and only in the case of Corfunc. The other parameters are always set to a certain value.
Is that not the case?
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.
Yep, I've got it now I've had a look at the utils. Given you have a set expectation of what the parameters should be I'll gently nudge you in the direction of my favourite python library pydantic . . .
If we have more dataclasses like these it may be worth us introducing it here down the line.
|
@DrPaulSharp thank you for your review. I have updated the slider input validation logic in both Invariant and Corfunc to account for edge cases and disable the buttons if the values are invalid. I have also rewritten the logic in Corfunc to match that of Invariant. So, instead of popping a dialog box on press, the button is just greyed out. What do you think of this? I will also look into Pydantic a bit more. |
DrPaulSharp
left a comment
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.
Hopefully we're nearly there, I've found one more problem. When you load data into Corfunc, unchecking the "Fit Guinier" checkbox (and other checkboxes) disables the "Go" button. However, when the checkbox is re-checked, the button is not re-enabled.
Description
This refactors the Corfunc slider into a reusable component so it can be shared by Invariant, making the slider generic and reusable for components that need different constraint semantics (e.g., Invariant needs to permit extrapolation beyond the raw data).
CorfuncSlider.pyout ofsrc/sas/qtgui/Perspectives/CorfuncExtrapolationSlider.pyand places insrc/sas/qtgui/Utilitiessrc/sas/sascalc/corfunc/calculation_data.pytosrc/sas/sascalc/util.pyBehavioural change (boundaries)
Introduces two distinct boundary pairs used by different callers:
Usage differences:
Corfunc: uses Data range only (no functional change except location/imports).
Invariant: uses both:
Fixes #3824
How Has This Been Tested?
Ran the program and tested the slider functionality in both perspectives.
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)