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

Constraints should be relational operator not assignment operators (Trac #487) #621

Closed
butlerpd opened this issue Mar 30, 2019 · 9 comments
Labels
Defect Bug or undesirable behaviour Major Big change in the code or important change in behaviour

Comments

@butlerpd
Copy link
Member

butlerpd commented Mar 30, 2019

currently the constraints/optimizer treats the constraint as an assignment rather than relational operator. The result is that for 3 models the following constraint set up

M1.parameter1=M2.paramter1
M1.parameter1=M3.paramter1

will result in ONLY the second constraint being implemented (the second assignment to M1.parameter1 overwrites the fist assignment). However if the constraints are set up as follows:

M2.parameter1=M1.parameter1
M3.parameter1=M1.parameter1

results in the expected behavior. This is both confusing to users and seems logically incorrect for a constraint which should always, I think, be a relational object.

Migrated from http://trac.sasview.org/ticket/487

{
    "status": "new",
    "changetime": "2018-07-05T09:33:19",
    "_ts": "2018-07-05 09:33:19.073889+00:00",
    "description": "currently the constraints/optimizer treats the constraint as an assignment rather than relational operator.  The result is that for 3 models the following constraint set up\n{{{\nM1.parameter1=M2.paramter1\nM1.parameter1=M3.paramter1\n}}}\nwill result in ONLY the second constraint being implemented (the second assignment to M1.parameter1 overwrites the fist assignment).  However if the constraints are set up as follows:\n{{{\nM2.parameter1=M1.parameter1\nM3.parameter1=M1.parameter1\n}}}\nresults in the expected behavior.  This is both confusing to users and seems logically incorrect for a constraint which should always, I think, be a relational object.",
    "reporter": "butler",
    "cc": "",
    "resolution": "",
    "workpackage": "Beta Approximation Project",
    "time": "2015-11-26T17:56:12",
    "component": "SasView",
    "summary": "Constraints should be relational operator not assignment operators",
    "priority": "major",
    "keywords": "",
    "milestone": "SasView Next Release +1",
    "owner": "",
    "type": "defect"
}
@butlerpd butlerpd added this to the SasView Next Release +1 milestone Mar 30, 2019
@butlerpd butlerpd added Beta Approximation Project Defect Bug or undesirable behaviour Major Big change in the code or important change in behaviour and removed Incomplete Migration labels Mar 30, 2019
@butlerpd
Copy link
Member Author

Trac update at 2016/03/20 22:51:55: butler changed milestone from "SasView 4.0.0" to "SasView Next Release +1"

@butlerpd
Copy link
Member Author

Trac update at 2017/10/27 10:32:32: butler changed milestone from "SasView Next Release +1" to "SasView 4.3.0"

@smk78
Copy link
Contributor

smk78 commented Mar 30, 2019

Trac update at 2018/07/03 13:25:28:

  • smk78 commented:

Moving to Beta Approximation Work Package whilst we consider constraints.

  • smk78 changed milestone from "SasView 4.3.0" to "SasView Next Release +1"
  • smk78 changed workpackage from "SasView Bug Fixing" to "Beta Approximation Project"

@RichardHeenan
Copy link
Contributor

Trac update at 2018/07/03 16:35:57: richardh commented:

I would say that constraints have to be "assignments" as the second example, they are after all mathematical equations. The first example does what it should with the scond over-riding the first. Suggest this ticket is closed.

@butlerpd
Copy link
Member Author

Trac update at 2018/07/04 19:02:24: butler commented:

While I agree that in constructing a full language the assignment approach would make sense, in this case it does not make sense other than to punish users who are not a programmer:-) I can see absolutely no situation where overriding makes any sense in the context of the constraints setup. If m1.x=m3.x why every say m1.x=m2.x??

We have however found users who want m2.x, m3.x etc to all be the fixed to m1.x ... and their natural behavior it appears is to write as example 1 above then don't understand why it doesn't work. I would argue that allowing overwriting adds unnecessary complexity to constraints since you have to now not only think about what relations you want but also worry about the order in which you provide them.

@RichardHeenan
Copy link
Contributor

Trac update at 2018/07/05 09:33:19: richardh commented:

We might perhaps introduce some new logic symbol such as M2.radius ~ M1.radius for something that has to work in both directions, but even then the details of how the fitting was done would have to be looked at very carefully. I suspect that sasview would still have to internally set up equivalent assignments.  Still suggest we close this ticket as we have plenty more to be getting on with.

[Actually in FISH, I can and do nest constraints, then tie back the least squares partial derviatives by running backwards through the constraints list.]

@lucas-wilkins
Copy link
Contributor

This is something we've picked out to fix here at some point

@lucas-wilkins
Copy link
Contributor

@gonzalezma to check progress.

@gonzalezma
Copy link
Contributor

Code now does not allow to have several definitions for the same parameter. Only the last definition is retained and shown in the list of constraints. This should already warn any inadvertent user that it is not possible to define two constraints as:
M1.parameter1=M2.parameter1
M1.parameter1=M3.parameter1

The way of defining a general constrain such as M1.p = M2.p = M3.p is:
M2.p = M1.p
M3.p = M1.p

Or entirely equivalent:
M1.p = M3.p
M2.p = M3.p

Or even:
M1.p = M2.p
M2.p = M3.p

While this is the expected behaviour for a programmer, it can be less intuitive for a non-programmer. The tutorial about the simultaneous fitting clarifies this, but perhaps a more detailed description should also be added to the documentation (at fitting_help.html#simultaneous-fits-with-constraints).

But there is nothing that can be easily done to change the "programmatic" behaviour, so closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Bug or undesirable behaviour Major Big change in the code or important change in behaviour
Projects
None yet
Development

No branches or pull requests

6 participants