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

vesicle @ structure_factor incorrect (Trac #1257) #233

Closed
pkienzle opened this issue Mar 30, 2019 · 5 comments
Closed

vesicle @ structure_factor incorrect (Trac #1257) #233

pkienzle opened this issue Mar 30, 2019 · 5 comments

Comments

@pkienzle
Copy link
Contributor

Comparing vesicle@S with no core to sphere@S I get different answers. Repeat using:

    $ sascomp vesicle@hardsphere,sphere@hardsphere radius=0,thickness thickness=40 -pars -midq -double

This is not true for hollow_cylinder with no core vs cylinder:

    $ sascomp hollow_cylinder@hardsphere,cylinder@hardsphere radius=0,thickness thickness=40 -pars -double

I think the issue is that the vesicle model defines volfraction, and this is colliding with the definition of volfraction in the structure factor, which then gets turned into volfraction_S.

Vescicle and sphere match if volfraction_S is set to the same value as volfraction.

    $ sascomp vesicle@hardsphere,sphere@hardsphere volfraction_S=0.05 scale=20,1 radius=0,thickness thickness=40 -pars -midq -double

Should update product model to use volfraction if it exists in the model.

This will require changes to sasmodels/product.py, both when constructing the model and when splitting the parameter set into P and S parameters.

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

{
    "status": "new",
    "changetime": "2019-03-25T21:00:59",
    "_ts": "2019-03-25 21:00:59.327232+00:00",
    "description": "Comparing vesicle@S with no core to sphere@S I get different answers.  Repeat using:\n{{{\n    $ sascomp vesicle@hardsphere,sphere@hardsphere radius=0,thickness thickness=40 -pars -midq -double\n}}}\n\nThis is not true for hollow_cylinder with no core vs cylinder:\n{{{\n    $ sascomp hollow_cylinder@hardsphere,cylinder@hardsphere radius=0,thickness thickness=40 -pars -double\n}}}\n\nI think the issue is that the vesicle model defines volfraction, and this is colliding with the definition of volfraction in the structure factor, which then gets turned into volfraction_S.\n\nVescicle and sphere match if volfraction_S is set to the same value as volfraction.\n{{{\n    $ sascomp vesicle@hardsphere,sphere@hardsphere volfraction_S=0.05 scale=20,1 radius=0,thickness thickness=40 -pars -midq -double\n}}}\n\nShould update product model to use volfraction if it exists in the model.\n\nThis will require changes to `sasmodels/product.py`, both when constructing the model and when splitting the parameter set into P and S parameters.\n",
    "reporter": "pkienzle",
    "cc": "",
    "resolution": "",
    "workpackage": "SasView Bug Fixing",
    "time": "2019-03-25T21:00:59",
    "component": "SasView",
    "summary": "vesicle @ structure_factor incorrect",
    "priority": "major",
    "keywords": "",
    "milestone": "SasView 4.3.0",
    "owner": "",
    "type": "defect"
}
@RichardHeenan
Copy link
Contributor

after merging https://github.com/SasView/sasmodels/tree/ticket_822_v5_unit_tests into master,
then vesicle@hardsphere still has issues due to collision of volfraction name?
In 5.0 the two volfraction are reporting same numbers, need to be able to keep them different and either of them fittable.

@pkienzle
Copy link
Contributor Author

You don't need to fit the two volfractions independently. Instead, fit scale and the tied volfraction, and interpret it as S.volfraction = tied volfraction, and P.volfraction = scale*S.volfraction.

Whether the 5.0 GUI should show the volfraction parameter in both models is a separate (and confusing!) issue. I suggest that reporting it as part of S and dropping it from P in the user interface since that is consistent with how all the models without volfraction operate. Or put "implicit volfraction" in all P models as a non-fittable parameter which shows the value from volfraction from S to make it clear that P@S is multiplied by volfraction even before scale is applied.

Bumps does allow you to compute uncertainty on derived parameters from the DREAM output. We could in principle show this on the implicit volfraction for P.

@RichardHeenan
Copy link
Contributor

What we have now in 5.0 is at least the same as 4.2 I(Q)=scale.Svolfrac.P(R,Q).S(Q,Reff,Svolfrac) if in some circumstances confusing for the users as scale changes on adding an S(Q). Suggest we leave this as is for now until we have agreement on whether all relevant P(Q) explicitly have a Pvolfraction, so that then I(Q)=scale.Pvolfrac.P(R,Q).S(Q,Reff,Svolfrac) when scale would not change on adding an S(Q).

Specifically for this ticket, in vesicle model there is a volfraction name collision which causes the previously mentioned bad behavior in 5.0 gui.
I locally tested a new version of vesicle where I renamed volfraction to volfractionshell which then of course worked perfectly. This could be a quick fix for vesicle, if the name change is put also into the loader for backwards compatibility, pending a more robust solution for an S(Q) parameter that has the same name as a P(Q) parameter.

@pkienzle
Copy link
Contributor Author

The relevant code is in ESS_GUI at
https://github.com/SasView/sasview/blob/f994f188e28dca36e7823b2deb3bf2bfc351c35c/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py#L2338

Simple fix is to pop the volfraction parameter out of s_params list before populating the s_params table.

Longer term, should add enough information into the model_info structure so that the GUI can group the parameters. This might be useful for standalone models with lots of parameters.

@ricleal ricleal transferred this issue from SasView/sasview Apr 23, 2019
@butlerpd
Copy link
Member

butlerpd commented May 1, 2019

With the merge of PR #101 this issue should now be closed

@butlerpd butlerpd closed this as completed May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants