Cyclic constraints inconsistent

[smk78]

Describe the bug
User IlonaS was trying to simultaneously fit two datasets in 5.0.6 to core_shell models with the following constraints:

M2.radius = M1.radius
M1.thickness = M2.thickness

This threw the following popup:

which seems reasonable.

BUG 1: This popup should declare whether it is an Error or a Warning.

Fearing the worst, the User started playing around to see if they could avoid the popup and found they could by setting these constraints instead:

M1.thickness = M2.thickness
M2.radius = M1.radius

BUG 2: This did not throw the popup!

Surely, if the first set of constraints are cyclic, the second set must be too???

SUPPLEMENTARY QUESTION: Are cyclic constraints 'bad' (and to be avoided, meaning the popup is an Error) or just 'not recommended' (meaning the popup is a Warning)?

To Reproduce
Run SasView.
Load 2 of the AOT test datsets and send them to fitting
Set both FitPages to core_shell models
Go in to the Constrained/Simultaneous Fit page and try setting the constraints above

Expected behavior
The popup should declare whether it is an error or warning.
The popup should appear for all instances of cyclic constraints.

SasView version (please complete the following information):

• Version: occurs in 5.0.6, and probably earlier, and 6.0.0

Operating system (please complete the following information):

• OS: reproduced on W10/x64

[gonzalezma]

Initially I could not reproduce this, but after reading more carefully the mails from Ilona I could reproduce it and I think that I understand what is happening. I think that this is more a misunderstanding on her part, and I don't consider that there is a real bug here. This is what I think is happening:

1. She has two sets and has initially declared the constraints as in the tutorial (M2.r = M1.r, M2.t = M1.t). This works and she should have proceeded to do the fitting.
2. But as the set that is more sensitive to the thickness is M2, she thinks that is better to declare M1.t = M2.t. There is no real reason for this, as it is a simultaneous fit, so it is not as we first fit M2 and then pass M2.t to M1. But it is not wrong either.
3. However, as she cannot declare this constraint using the menu, because M1.t is not a free parameter any more and therefore it does not appear in the list of available parameters, she edits the constraint and swaps M1 and M2. My guess is that here, after pressing enter, the first action is to check the new constraint, but the previous one has not yet been removed, so at this point we still have 2 definitions: M1.t = M2.t, M2.t = M1.t, and hence the cycling error.

I guess we could store the previous definition, then delete it from the active constraints, check the new definition and, if correct, use it to replace the previous one and, if not, recuperate the previous one. But is it worthy to add so much complication? Personally, I would say no.

[smk78]

Ahh, thanks @gonzalezma .
One of the edits made to the v6 simultaneous fitting tutorial (at your request I believe) that was back-ported to the v5 tutorial was to remove the wording about choosing the RHS to be the model that was most sensitive to the parameter being constrained. So maybe that will help stop a User from encountering this again (although in all the years that tutorial has been out there no one has reported this before!).

[smk78]

So, just to summarise, I think this means:

BUG 1: Should be addressed and the word Warning added to the popup (else it looks like an error and confuses the User).
BUG 2: Is not a bug.
SUPPLEMENTARY QUESTION: They're not recommended.

Anyone disagree?

[butlerpd]

Isn't the cyclic pop-up and error rather than a warning? In other words it should not be able to actually fit properly right? Or did I misunderstand? cause if it doesn't matter why worry the user with a pop up at all?

I agree that bug 2 is not a bug. In principle the two parameters are independent so it should work even if it will get confusing and quickly lead to real cyclical problems if they continue doing that? So I would generally recommend not doing that I guess? Are you asking about whether that should be added to the tutorial?

[smk78]

@Butler wrote:

Isn't the cyclic pop-up and error rather than a warning? In other words it should not be able to actually fit properly right? Or did I misunderstand? cause if it doesn't matter why worry the user with a pop up at all?

Well, @gonzalezma describes the behaviour as "no real reason for this, ... But it is not wrong either" which doesn't ooze Error to me? However, your second question, why worry the user at all, is perhaps the more pertinent!

So I would generally recommend not doing that I guess? Are you asking about whether that should be added to the tutorial?

No. And as mentioned in #3125 (comment), the wording that led Ilona down this rabbit hole has been changed in the latest versions anyhow.

So close this issue with no action, or make the popup a warning first?

[gonzalezma]

The cycling error was added to avoid having the following situation (e.g. with just 2 parameters A and B):
A=B
B=A

I think this was giving an error because there is no "free" parameter to fit any more (but I may be wrong here, so it would be good to check with PK). So at the time, Nico (I think) implemented two features:

1. When a parameter is constrained, it disappears from the menu of available parameters to be used on the right side of the equation. So it is not possible to define the previous 2 constraints using the menu.

2. Whenever a constraint is edited, after pressing enter there is a call to a function to check it, e.g. that the parameters used correspond to true model parameters (e.g. no typos or a parameter thickness in a model that it has not thickness) and that a cycling dependence has not been introduced.

So I would say that what to do depends on how bumps will handle the cyclic constraint. If it is able to understand the double definition (A=B + B=A) and do the expected thing (do the fit with a single common parameter for A and B), we could remove the check for the cycling error and ignore the redundancy in the user's definition. If not, we still need it.

As for Ilona's case, the problem is not that she used A=B or B=A, which both are equivalent, but that in the moment of checking the edited constraint we have both "in memory". But of course she could not know this and I understand that the observed behaviour can be very confusing. But only if PK confirms that bumps will handle this situation correctly, we should remove the cycling error.