More changes to corfunc

[lucas-wilkins]

Description

Multiple changes to corfunc

1. Restructured the corfunc interface, now a single click to do everything, with options
2. Backend refactored
3. Fixed what appears to be a longstanding bug in the corfunc guinier fitting code
4. Background warning only shows if the extrapolated function goes below zero
5. Fixed text overlap, probably (see comments about general Qt font issues, Text formatting in Corfunc Perspective needs attention #2456)

Single command might method could potentially be slow on older machines, its fine on mine, but if it's irritatingly slow, might need to add some hints so the user can see its working.

Fixes #2458
Fixes #2456
Fixes #2454

Contains commits from #2457 and so also
Fixes #2453

How Has This Been Tested?

Basic functionality tests, unit tests updated and pass

Review Checklist (please remove items if they don't apply):

• Code has been reviewed
• Functionality has been tested
• Windows installer (GH artifact) has been tested (installed and worked)
• MacOSX installer (GH artifact) has been tested (installed and worked)
• User documentation is available and complete (if required)
• Developers documentation is available and complete (if required)
• The introduced changes comply with SasView license (BSD 3-Clause)

[smk78]

Tested on W10/x64 using artefact https://github.com/SasView/sasview/actions/runs/4416180283.

Overall, I like this reworking! And it seems to be producing transforms much more akin to what the original Corfunc did! 😄

But a few comments:

• the limit sliders seem much more sluggish than before; they are certainly much less interactive.
• when I move sliders I get stuff like this in the Log Explorer:

16:51:31 - ERROR: Traceback (most recent call last):
  File "sas\qtgui\Perspectives\Corfunc\CorfuncPerspective.py", line 632, in on_extrapolation_slider_changing
  File "sas\qtgui\Perspectives\Corfunc\QSpaceCanvas.py", line 38, in update_lines
TypeError: 'ExtrapolationParameters' object is not subscriptable

• If you click Go and get a result, but then go and move the sliders or change radio buttons, and then click Go again, there can be a delay of a couple of seconds before the operation completes. The problem with this is that unless some extracted parameters actually change, it is very difficult to tell that anything has happened! I think there is a slight change of shading on the Go button but it's not very obvious. Can it be made more obvious that it is calculating?
• Having performed a transform with a double extrapolation (ie, Fit Guinier and Fit Porod checked) I then unchecked them but the Q Space graph never changed (the parameters did change).
• And now this is a one-button-does-all operation, I'm less keen on the plot view automatically switching to the Extraction Diagram, because you need to check the extrapolations are sensible!!!

I think there was a comment on an issue that maybe there should be something that tells you how good the extrapolations are. I would agree. But I'm not sure what...!

[butlerpd]

Yes @lucas-wilkins it is a LOT less cluncky looking or feeling. My only comments would be (and keep in mind it was a very quick play and I don't really use that type of analysis)

• The default plot seems to be an odd choice I would think the Real Space one might be the most useful generally though even the first Q space one would be a better default for me. @smk78 would know much better however..
• It did seem a bit slow (nothing happens for a while which is always a bit unnerving 😃 and I think my computer is actually a solid mid range speed (maybe even high middle?)
• Finally The default window height is sized for the graph but cuts off the bottom 3rd of the output text so I "had" to immediately stretch the window (or scroll of course but I only want to do that if necessary)? Maybe this is the least bad option to have something legible on a laptop? And certainly if keeping the height down is needed then the layout is done quite nicely to not make it too irritating. Still, I thought I'd mention that I noticed it on a first go 😄

Since I am neither an expert in this type of analysis nor did I look at the code I will submit this as a review comment without approving (or asking for changes)

[butlerpd]

Indeed ... clearly I did not even play with the sliders ... I could not get them to move at all.

[lucas-wilkins]

TypeError: 'ExtrapolationParameters' object is not subscriptable

Aha, I missed one.

[lucas-wilkins]

If you click Go and get a result, but then go and move the sliders or change radio buttons, and then click Go again, there can be a delay of a couple of seconds before the operation completes. The problem with this is that unless some extracted parameters actually change, it is very difficult to tell that anything has happened! I think there is a slight change of shading on the Go button but it's not very obvious. Can it be made more obvious that it is calculating?

That's what I was asking about with the slowness thing. I'll come up with a solution

[lucas-wilkins]

And now this is a one-button-does-all operation, I'm less keen on the plot view automatically switching to the Extraction Diagram, because you need to check the extrapolations are sensible!!!

Yeah, I was unsure about that.

[lucas-wilkins]

If you click Go and get a result, but then go and move the sliders or change radio buttons, and then click Go again, there can be a delay of a couple of seconds before the operation completes. The problem with this is that unless some extracted parameters actually change, it is very difficult to tell that anything has happened! I think there is a slight change of shading on the Go button but it's not very obvious. Can it be made more obvious that it is calculating?

A thing that complicates this is that its actually the plotting in matplotlib that is taking ages, not the calculation. It's much harder to get the appropriate feedback from the plots.

[lucas-wilkins]

@smk78 Everything you mentioned should be addressed now.

[smk78]

Tested on W10/x64 using artefact https://github.com/SasView/sasview/actions/runs/4429287263.

• The sliders are much more interactive again! 👍
• There is no longer an error message when you move the sliders 👍
• The Go button now displays Calculating... when it's doing something 👍
• And the plot view now stays on the Q Space 👍

[smk78]

The only one of my earlier comments (in this PR) that does not seem to have been adressed is the one regarding the behaviour when you uncheck/check the Fit Guinier and Fit Porod boxes.

It may well be that these extrapolations are not/are being used in the behind-the-scenes calculations depending on the settings, but the Q Space plot always shows both extrapolations whatever. Which is 'unnerving'.

[lucas-wilkins]

@smk78 Thanks for doing this so quickly.

It may well be that these extrapolations are not/are being used in the behind-the-scenes calculations depending on the settings, but the Q Space plot always shows both extrapolations whatever. Which is 'unnerving'.

It does what it should. The extrapolations need to happen at both ends whether you fit the parameters or not. The boxes control whether they are fitted, allowing you to manually enter a number in the background/guinier/porod parameter fields. If they are ticked. the corresponding text fields will be overwritten by the relevant fit values. This functionality works AFAICT.

Misunderstanding of what the checkboxes should do.

[smk78]

It does what it should. The extrapolations need to happen at both ends whether you fit the parameters or not. The boxes control whether they are fitted, allowing you to manually enter a number in the background/guinier/porod parameter fields. If they are ticked. the corresponding text fields will be overwritten by the relevant fit values. This functionality works AFAICT.

Oh, I see... Then I would argue the behaviour is a little unintuitive.
When you first enter the perspective the extrapolation parameter boxes are blank. If you untick the fit boxes at this point it is unclear whether any extrapolation will be performed or, if it is (as actually happens), what parameters will be used. Only when you click Go do you find it uses zeroes. But conversely, if the fit boxes are checked the parameter boxes are still editable!
I contend the boxes should be initialised with zeroes, and that if the fit boxes are checked the parameter boxes should be non-editable? Doing this would then help make it obvious that if you do a calculation with fitted parameters and then uncheck the fit boxes you are actually doing something, so that you shouldn't be surprised if you click Go again and get the same result! 😄

[smk78]

How has #2454 been addressed in this PR? The export transformed output seems to be the same?

[lucas-wilkins]

I renamed save to export.

…

On Thu, Mar 16, 2023 at 6:09 PM Steve K ***@***.***> wrote: How has #2454 <#2454> been addressed in this PR? The export transformed output seems to be the same? — Reply to this email directly, view it on GitHub <#2463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACKU4SQY3JD6F6JZFYUNUU3W4NJPJANCNFSM6AAAAAAV2OASQM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Dr Lucas Wilkins +44 (0) 7505 915 726 Personal Website: http://www.lucaswilkins.com/ Alternate e-mail: ***@***.***

[krzywon]

A couple of suggestions, but the code looks good. I haven't tested the functionality.

[lucas-wilkins]

I contend the boxes should be initialised with zeroes

Potentially.

and that if the fit boxes are checked the parameter boxes should be non-editable?

I was considering doing the opposite, which was to have it so that if you manually change the text, tick box becomes unticked.

Think this is for the next round of changes though.