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.

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

1728: Finishing the Q-Range Sliders #1891

Merged
merged 28 commits into from
Oct 12, 2021
Merged

1728: Finishing the Q-Range Sliders #1891

merged 28 commits into from
Oct 12, 2021

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Aug 10, 2021

This finishes the work related to the Q-range sliders introduced in v5.0.4.

  • The sliders are still defined in the Data1D class, but the values are now pickle friendly.
  • The invariant sliders are now reinstated and are properly linked between the Npts and min/max Q inputs.
  • The linear fit window shows a pair of sliders that are linked to the data set. These lines are removed from the plot when the linear fit window is closed.
  • The visibility of the sliders can be toggled on/off from the context menu. The visibility should be retained between plot updates (fits, etc.).
  • The QRangeSlider class is now documented.
  • Unit tests were added to ensure perspectives and sliders are linked properly without changing the results of existing unit tests.

Fixes #1728

…hed editing, and fix bug related to number of points calculation for high-q extrapolation.
… values instead. Use getattribute to assign unpickleable objects within the plotter objects.
…nearFit tests fully working, and Fitting tests started (but not working). Added the tests to the plottingSuite in GUITests.py
…se global to link perspective info to QRangeSlider
@smk78 smk78 requested review from smk78 and rozyczko September 1, 2021 12:57
Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

Observations regarding the sliders on a linear fit:

  • if you just load some data and plot it (not send it to fitting) no sliders are visible as expected. If you then use the context menu to request a linear fit on the data the sliders appear but are not draggable, they can only be repositioned by typing x values in the linear fit box. This is ok, I think, once you realise.
  • you can then toggle those sliders off. But if you do so, then toggle them back on, they reset to the full range of the data, not where they were. And similarly the x values in the linear fit box also reset. I thought this was a bit annoying. Would it be difficult to keep some 'memory' of the last positions?
  • if you send the data to fitting instead, then of course the plot will have the data and the model calculation and the sliders that appear by default are connected to the calculation. If you now request a linear fit of the data (not calculation) you get another set of sliders! Then things get a bit weird! If you put some x values in the linear fit box that are different to the slider limits on the calculation the new (data) sliders initially move but then jump back to the full range. Further changes of the x values then don't result in any further movement of the new (data) sliders unless you close the linear fit box and reopen it.

Observations regarding the sliders on the invariant extrapolation:

  • if you enable high & low-Q extrapolations you get six sliders; the start & finish of the data, the start & finish of the low-Q extrapolation, and the start & finish of the high-Q extrapolation! All these sliders are the same colour and two almost overlap by default!
  • It is unclear to me what purpose having the sliders for the start & finish of the data serves. As far as I am aware (but am happy to be corrected), the invariant calculation perspective always integrates all the loaded dataset; the only thing the user has control of is how much of either extrapolation is also included?
  • in this respect, moving the data-most slider for either extrapolation updates the Npts number for that extrapolation; at some point (in a separate ticket!) we probably ought to display Q-ranges in the Invariant GUI, and possibly also give control over the number of points in either extrapolation?
  • If it is indeed the case that you can select how much of the loaded data is integrated, then those limits definitely need to be displayed on the GUI somewhere. I would actually argue that this would be a useful enhancement of the analysis (but again would be a separate ticket of course!).

@krzywon
Copy link
Contributor Author

krzywon commented Sep 2, 2021

Thanks for testing this out and the comments, @smk78. I've added some notes below.

  • if you just load some data and plot it (not send it to fitting) no sliders are visible as expected. If you then use the context menu to request a linear fit on the data the sliders appear but are not draggable, they can only be repositioned by typing x values in the linear fit box. This is ok, I think, once you realise.
  • you can then toggle those sliders off. But if you do so, then toggle them back on, they reset to the full range of the data, not where they were. And similarly the x values in the linear fit box also reset. I thought this was a bit annoying. Would it be difficult to keep some 'memory' of the last positions?

The linear fit window is modal so you can't interact with the plot when the window is open. The sliders are supposed to be wiped away when the linear fit window closes, so the toggle should not be available. I've found ways to repeat the behavior you're seeing and already have this fixed locally.

Every time the linear fit window is opened, a new instance of the window is created, causing the x values to reset. I could potentially change the behavior to hide/show the window instead, but that is beyond the scope of this PR, and might cause odd interactions in other places.

  • if you enable high & low-Q extrapolations you get six sliders; the start & finish of the data, the start & finish of the low-Q extrapolation, and the start & finish of the high-Q extrapolation! All these sliders are the same colour and two almost overlap by default!
  • It is unclear to me what purpose having the sliders for the start & finish of the data serves. As far as I am aware (but am happy to be corrected), the invariant calculation perspective always integrates all the loaded dataset; the only thing the user has control of is how much of either extrapolation is also included?

I am unable to reproduce this behavior after fixing the linear fit sliders. Did you do a linear fit on the same set of data beforehand? Maybe the sliders were erroneously still around. Do you want me to push my fix so you can test again?

  • in this respect, moving the data-most slider for either extrapolation updates the Npts number for that extrapolation; at some point (in a separate ticket!) we probably ought to display Q-ranges in the Invariant GUI, and possibly also give control over the number of points in either extrapolation?
  • If it is indeed the case that you can select how much of the loaded data is integrated, then those limits definitely need to be displayed on the GUI somewhere. I would actually argue that this would be a useful enhancement of the analysis (but again would be a separate ticket of course!).

The more I work with the invariant, the more I think we need to differentiate the extrapolated q-range from the fitted q-range a lot better. A single ticket might be able to incorporate both of your points.

@smk78
Copy link
Contributor

smk78 commented Sep 2, 2021

Yes, push your fix @krzywon and I'll retest.

The more I use the Invariant, the more I think it needs a complete reimagination! :-)

Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

I've just done a functionality review on the commit cace230 and behaviour is now a lot cleaner:

  • the sliders are removed when the linear fit box is closed;
  • and now you only get four sliders (not six!) when using the invariant panel, regardless of whether you send data straight to invariant analysis or have done a fit on it first.

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Sep 27, 2021
@wpotrzebowski
Copy link
Contributor

@smk and @rozyczko to look at it

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Sep 28, 2021
@rozyczko
Copy link
Member

rozyczko commented Oct 1, 2021

This version still reacts to typing values in Min/Max range textboxes, rather than validate the values on textbox exit. Also, moving the bars seems very sluggish and probably has to do with on-move update of these textboxes.
I think this update should be done once, on mouse button release.

@smk78
Copy link
Contributor

smk78 commented Oct 1, 2021

As there have been no further commits to this branch since my previous review/approval I have nothing further to add. But I note @rozyczko has made a comment.

@krzywon
Copy link
Contributor Author

krzywon commented Oct 5, 2021

I suppressed plot redraws while the sliders are moving and used the editingFinished connection instead of the textChanged to reduce the sluggishness, and the response is now much faster. I merged main into my branch as well so the Fitting changes from #1798 would be incorporated, but that means fitting is not working properly, as noted in #1914.

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 12, 2021
@butlerpd
Copy link
Member

@rozyczko agrees that concerns are fixed and ready to merge

@butlerpd butlerpd merged commit 04dfe62 into main Oct 12, 2021
@krzywon krzywon deleted the slider-final-work branch November 9, 2021 19:07
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vertical bars for q-range
5 participants