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

Sum/Product Models don't do what they should (Trac #767) #881

Closed
smk78 opened this issue Mar 30, 2019 · 14 comments
Closed

Sum/Product Models don't do what they should (Trac #767) #881

smk78 opened this issue Mar 30, 2019 · 14 comments
Assignees
Labels
Critical High priority Defect Bug or undesirable behaviour
Milestone

Comments

@smk78
Copy link
Contributor

smk78 commented Mar 30, 2019

The Sum|Multi model generator is not creating properly parameterised models:

'''Sum'''
Should do: scale_factor * {(scale_1 * model_1) + (scale_2 * model_2)} + background

Is doing : scale_factor * {(scale_1 * model_1 + background_1) + (scale_2 * model_2 + background_2)}

'''Product'''

Should do: scale_factor * model_1 * model_2 + background

Is doing : (scale_1 * model_1 + background_1) * (scale_2 * model_2 + background_2) + background

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

{
    "status": "closed",
    "changetime": "2017-09-15T15:38:14",
    "_ts": "2017-09-15 15:38:14.545678+00:00",
    "description": "The Sum|Multi model generator is not creating properly parameterised models:\n\n'''Sum'''\nShould do: scale_factor * {(scale_1 * model_1) + (scale_2 * model_2)} + background\n\nIs doing : scale_factor * {(scale_1 * model_1 + background_1) + (scale_2 * model_2 + background_2)}\n\n'''Product'''\n\nShould do: scale_factor * model_1 * model_2 + background\n\nIs doing : (scale_1 * model_1 + background_1) * (scale_2 * model_2 + background_2) + background\n",
    "reporter": "smk78",
    "cc": "",
    "resolution": "fixed",
    "workpackage": "SasView Bug Fixing",
    "time": "2016-10-10T21:30:14",
    "component": "SasView",
    "summary": "Sum/Product Models don't do what they should",
    "priority": "critical",
    "keywords": "",
    "milestone": "SasView 4.2.0",
    "owner": "lewis",
    "type": "defect"
}
@smk78 smk78 added this to the SasView 4.2.0 milestone Mar 30, 2019
@smk78 smk78 added Critical High priority Defect Bug or undesirable behaviour Incomplete Migration and removed Incomplete Migration labels Mar 30, 2019
@smk78
Copy link
Contributor Author

smk78 commented Mar 30, 2019

Trac update at 2016/10/10 21:34:41: smk78 changed description from:

The Sum|Multi model generator is not creating properly parameterised models:

'''Sum'''
Should do: scale_factor * {(scale_1 * model_1) + (scale_2 * model_2)} + background

Is doing : scale_factor * {(scale_1 * model_1 + b/g_1) + (scale_2 * model_2 + b/g_2)}

'''Product'''

Should do: scale_factor * model_1 * model_2 + background

Is doing : (scale_1 * model_1 + b/g_1) * (scale_2 * model_2 + b/g_2) + background

to:

The Sum|Multi model generator is not creating properly parameterised models:

'''Sum'''
Should do: scale_factor * {(scale_1 * model_1) + (scale_2 * model_2)} + background

Is doing : scale_factor * {(scale_1 * model_1 + background_1) + (scale_2 * model_2 + background_2)}

'''Product'''

Should do: scale_factor * model_1 * model_2 + background

Is doing : (scale_1 * model_1 + background_1) * (scale_2 * model_2 + background_2) + background

@butlerpd
Copy link
Member

Trac update at 2016/10/11 18:20:59:

  • butler changed owner from "" to "ajj"
  • butler changed status from "new" to "assigned"

@butlerpd
Copy link
Member

Trac update at 2017/03/06 03:59:13: butler commented:

Is this really a blocker? Obviously the change would be much cleaner but not sure it is a blocker. By appropriate choices of values for scales and background the expected behavior can be achieved, just a bit .... ugly!

@smk78
Copy link
Contributor Author

smk78 commented Mar 30, 2019

Trac update at 2017/03/06 10:17:55: smk78 commented:

Except no user actually knows that... :-)

@butlerpd
Copy link
Member

Trac update at 2017/03/07 02:20:16: butler commented:

Welll... funny you should say that:-) Actually I would argue it will be quickly obvious to most "ordinary" users and that the people who will be most confused will be us, the developers. Indeed we have been talking so long about what we want it to be but cant keep track in the flurry of activities what has and has not been done that we (at least I do) get confused.

In fact, the very title of the ticket is incorrect. Sum and multiply do EXACTLY what they say. In fact the statement of the problem is a bit misleading. For example for the sum model it says:

Is doing : scale_factor * {(scale_1 * model_1 + background_1) + (scale_2 * model_2 + background_2)}

But really what it is doing is exactly what it says when you create the model namely:

Scale_factor * (Model_1 + Model_2).

All the parameters of model 1 and all the parameters of model 2 are present (and labeled as 1 or 2) and a separate non numbered scale factor is added. If the model in question has a scale and/or a background then it/they will show up, and if not (try for example hard sphere as one of the models) it/they won't. An ordinary user might asks us: what part of model_1 don't you guys understand? For me it would be the fact that when writing models I don't usually include scale and background anymore which are auto added (or not as the case may be), but an ordinary user will likely not be so encumbered by prejudice :-)

That said I think we should change the behavior as soon as we can as suggested in the ticket. I am just worried that we may not have the bandwidth to get it done soon enough for a release and questioning the priority of this particular ticket.

@butlerpd
Copy link
Member

Trac update at 2017/03/07 13:42:13: butler commented:

ooops my bad. While last comment I think is correct, my previous one is not entirely so. Mainly for a sum model there is no way to decouple scale(time del rho etc) from background.

@ajj
Copy link
Member

ajj commented Mar 30, 2019

Trac update at 2017/03/16 20:23:38:

  • ajj commented:

Checked current master. Changed name of ticket to reflect real nature of ticket.

This should be fixed as the behaviour is a pain - all the multiplication of backgrounds and the "!BackGround" parameter with inconsistent capitalisation is horrid.

Should probably be replaced by something nicer in sasmodels that can properly cope with more arbitrary model arithmetic, but behaviour could also be fixed in current implementation with suppression of background parameters etc.

Not a blocker for 4.1

  • ajj changed milestone from "SasView 4.1.0" to "SasView 4.2.0"
  • ajj changed priority from "blocker" to "major"
  • ajj changed summary from "Sum/Product Models don't do what they claim" to "Sum/Product Models don't do what they should"

@butlerpd
Copy link
Member

Trac update at 2017/04/10 09:58:03: butler changed priority from "major" to "critical"

@ajj
Copy link
Member

ajj commented Mar 30, 2019

Trac update at 2017/08/30 13:29:20: ajj changed owner from "ajj" to "lewis"

@ajj
Copy link
Member

ajj commented Mar 30, 2019

Trac update at 2017/08/30 13:30:19: ajj commented:

The model arithmetic should make use of the tools in sasmodels that Paul K has worked on.

@sasview-bot
Copy link

Trac update at 2017/09/13 15:33:27: lewis commented:

The sasmodels code for this was merged into master in commit [https://github.com/SasView/sasmodels/commit/b18e6504c1a437f9a7b2fdf79df8d691cbbd6a9d b18e650]

@smk78
Copy link
Contributor Author

smk78 commented Mar 30, 2019

Trac update at 2017/09/15 10:21:45: smk78 commented:

The documentation for custom models will probably need updating too. Now if a user creates a sum/multi model, a file that looks like the one below will be created in the plugin_model directory:

from sasmodels.sasview_model import make_model_from_info
from sasmodels.core import load_model_info

model_info = load_model_info('sphere+cylinder')
model_info.name = 'MyModel'
Model = make_model_from_info(model_info)

By changing the string that's the argument to the load_model_info function, the user can change the model. Any combination of model names, + and * will work. The use of brackets isn't supported, and operations will be executed in the correct order (i.e. multiplication first, then addition). Custom model names can also be used in the model string by prepending the model name with 'custom.', e.g. sphere+custom.!MyModel

Structure factor models can also be created using the @ symbol, e.g. cylinder@hardsphere

@smk78
Copy link
Contributor Author

smk78 commented Mar 30, 2019

Trac update at 2017/09/15 15:37:56: smk78 commented:

Lewis' Ticket-767 branch has now been merged to master and the docs updated so I am closing this ticket.

Related ticket 861 (and associated pull request 104) could do with a second review just in case there is a sasmodels dependency that has been missed.

@smk78
Copy link
Contributor Author

smk78 commented Mar 30, 2019

Trac update at 2017/09/15 15:38:14:

  • smk78 changed resolution from "" to "fixed"
  • smk78 changed status from "assigned" to "closed"

@smk78 smk78 closed this as completed Mar 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical High priority Defect Bug or undesirable behaviour
Projects
None yet
Development

No branches or pull requests

5 participants