Improvements to Corfunc

[lucas-wilkins]

Description

Various improvements to corfunc:

1. More robust calculation of inflection point, existing method was numerically unstable
2. Added different ways of extracting parameters
3. Added a diagram showing how the parameters are calculated (like in Fibre_Diffraction_Review_1994_3_25-29)
4. Bug fix: extrapolation range was incorrect

Fixes #2444
Fixes #2445

How Has This Been Tested?

Tested on a couple of example data files.

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]

Testing artefact 4325037288 on W10/x64.

I think this is looking good.

I would still very much prefer to have draggable limit bars on the plot itself, like in the fitting perspective, and not the Adjust slider.

When you click Save Extrapolated, the dialog box that appears is populated with the limits of the input data. This seems unintuitive to me, particularly since the Q Space plot is displaying an actual extrapolation beyond the ranges of the input data. Can we populate the dialog with some extrapolated limits instead, please?

We also need to have a think about the action of the Save Transformed button. I appreciate it is currently writing the same output that @rprospero implemented, namely a 4-column CSV (X, 1D CF, 3D CF & IDF). But as @butlerpd pointed out on an issue, if you then try and use this file in SasView it loads X vs 1D CF and applies the 3D CF values as uncertainties! And the IDF is ignored. I can think of two ways around this. One is to always simply write 3 files, appending to the user-provided filename _1D, _3D and_IDF for X vs 1D CF, X vs 3D CF and X vs IDF, respectively. The other is to have a radio button to select what transform you want to output. I think I lean towards the former solution.

As far as I can see, the parameter part of the perspective is of fixed width; you can drag the who perspective wider, but all that does is stretch the plots, you can't widen the parameter part? The reason I was trying to do this is that on my monitor (standard 1920x1080 widescreen) the text for some of the Lamellar Parameter boxes is 'lost':

I do like the Extraction Diagram!

On the Real Space (& Extraction Diagram) if the Y-axis label said 'Correlation Function' we could drop 'Correlation' from the curve legend.

But...

We still have a big problem with the parameter extraction. Transforming the example data ISIS_98929.TXT you get something like this:

Clearly, the 'true first maxima' lies around 75 Ang, but the extraction process is focussing on the wiggle about 30 Ang:

These data are from polyamide-6 so the actual long period is known (between ~60 - ~75 Ang). So at the moment the perspective is returning an inaccurate estimate. This is further reinforced by simple inspection of the position of the peak in the Q Space data: around 0.067 /Ang. Doing 2.pi/0.067 gives ~ 93 Ang. Almost 3 times the long period being returned at present.

The Report is good.

It doesn't appear you can copy the parameters in the perspective or save them to file from the Edit menu? Is that WiP?

Obviously the User docs will need an update. :-) On me I guess!

[smk78]

See preceding comment for a functionality review.

[lucas-wilkins]

OK, no changes to make on this PR then.

[lucas-wilkins]

Clearly, the 'true first maxima' lies around 75 Ang, but the extraction process is focussing on the wiggle about 30 Ang

So, do we want an option to choose first maximum or largest nonzero maximum? is it always the largest that we want?

Anyway, new issue for that.

[smk78]

OK, no changes to make on this PR then.

Ok. Sorry. I still haven't got used to your rigorous 1 PR = 1 issue approach! 😄 I'll change my review to approve. But I've not reviewed the code.

[smk78]

Functionally does what it advertises. Have not reviewed the code.

[smk78]

Clearly, the 'true first maxima' lies around 75 Ang, but the extraction process is focussing on the wiggle about 30 Ang

So, do we want an option to choose first maximum or largest nonzero maximum? is it always the largest that we want?

Anyway, new issue for that.

It should, I think, always be the largest positive maximum. If the background correction has been applied correctly. Ok, I'll create a new issue for this.

[butlerpd]

Do we need a code review? what about OSX (I assume @smk78 only tested the artifact on windows?) Based on this review I will check off functionality and windows testing. Does this require any changes to documentation?

[lucas-wilkins]

@butlerpd There will be a need for documentation changes in due course, but there are lot more changes to go before then.