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

5.0 volfraction in S(Q) #232

Open
RichardHeenan opened this issue Apr 1, 2019 · 21 comments
Open

5.0 volfraction in S(Q) #232

RichardHeenan opened this issue Apr 1, 2019 · 21 comments

Comments

@RichardHeenan
Copy link
Contributor

I am opening a new ticket for this, though it has in part been covered by a number of other tickets related to beta approx project and P(Q)S(Q) (most of which can now be closed).

The radius_effective and beta approx parts now work, though as usual a lot more unit tests are needed. [Also some issues with vesicle mode.]

The final part of the "beta approx, plus improve S(Q)" project was not started as the UK student, Torin, ran out of time. It was intended that we sort out the the use of volfraction and scale when using S(Q).

What we have currently in v5.0 regarding scale & volfraction is the same as v4.2 , without S(Q) then:
I(Q) = scale.P(radius,Q) where scale is volume fraction if working in absolute units
with S(Q) then:
I(Q) = scale.Svolfrac.P(radius,Q).S(Q,radius_effective,volfraction_effective)

This is confusing for users as the value and meaning of scale changes on adding an S(Q), it would be much better is this did not happen.

We should in v5.0 add a third drop down in the fitting gui to choose whether to constrain volume fraction in S(Q) or let it freely adjust, i.e. volfraction_effective_mode=1 for constrained, 0 for freely adusting. The drop down could appear alongside the one for radius_effective_mode.

It has been suggested that any P(Q) which can be multiplied by an S(Q) should have an explicit volfraction parameter added, but I don't think that is actually necessary.

I would propose - I(Q)=scale.P(radius,Q).S(Q,radius_effective,volfraction_effective)

Where if working in absolute units, then just as in v4.2, scale = phi the volume fraction of particles, but now Svolfrac is only affects the shape of S(Q) and not the overall height of I(Q).

In the main fitting gui selecting volfraction_effective_mode=1, constrains Svolfrac = scale
volfraction_effective_mode=0, allows Svolfrac to be fitted

In sum/multi plugin models then volfraction_effective_mode defaults to 0, exactly as v4.2

If loading a P(Q)S(Q) from sasview 4, then scale has to be set to scale_v4*volfraction

I think that this should be done before releasing 5.1 to the general public to avoid confusion later.

Would this work ? - your opinions please.

We could instead add volfraction explicitly to all P(Q) models that can take an S(Q), so that

I(Q)=scale.volfraction.P(R,Q,volfraction).S(Q,Reff,Svolfrac)

But I don't think that there any P(Q) that actually need volume fraction as a genuine parameter ?

NOTES
(i) volfraction_mode = 1 requires special constraints as v4.2 for vesicle and hollow_cylinder models which are scaled by the shell rather than exterior volume
(ii) Some changes would be required to the results checks of the new @s unit tests, and
(iii) likely also to sasfit_compare_new.py

@pkienzle
Copy link
Contributor

pkienzle commented Apr 1, 2019

See discussion on #609.

@pkienzle
Copy link
Contributor

pkienzle commented Apr 3, 2019

If the usual case is to decouple S.volfraction and P.volfraction, then renaming volfraction to volfraction_effective for the structure factor is a reasonable option. If they are usually constrained to be equal but are sometimes different then leaving the existing code in place will be more convenient for the user.

We could create volfraction_mode like we do for radius_effective_mode but this adds an extra knob in the interface (so makes the program harder to use) which is only fittable if volfraction_mode is 0 (so extra GUI code to maintain). Only case where it makes a difference is when constraining S.volfraction = P.volfraction * <V_outer>/<V_shell> in the vesicle model. Do we really need this constraint? If not then we can simplify a bunch of code which is currently tracking enclosed volume separately from shell volume.

Note: if renaming volfraction to volfraction_effective, remember to update the conversion code so that old P@S models can be reloaded. Can either rename it in the individual structure factor models or when composing the P@S model.

@smk78
Copy link
Contributor

smk78 commented Apr 3, 2019

Richard says:

I would propose - I(Q)=scale.P(radius,Q).S(Q,radius_effective,volfraction_effective)

Where if working in absolute units, then just as in v4.2, scale = phi the volume 
fraction of particles, but now Svolfrac is only affects the shape of S(Q) and not 
the overall height of I(Q).

In the main fitting gui selecting volfraction_effective_mode=1, constrains 
Svolfrac = scale and volfraction_effective_mode=0, allows Svolfrac to be fitted

I am confused. Do you mean volfraction_effective, instead of Svolfrac?

@RichardHeenan
Copy link
Contributor Author

Yes Svolfrac is of course volfraction_effective but quicker to type.

@RichardHeenan
Copy link
Contributor Author

I am tempted to go along with what pkienzle has said above, change to
I(Q)=scale.P(radius,Q).S(Q,radius_effective,volfraction_effective)

and NOT provide any built in constraints on volfraction_effective.

If the user is in absolute units then they can add one of our standard constraints to do volfraction_effective = scale, or if in arbitrary units then include their own rescale factor e.g. volfraction_effective = 1.234e-03*scale again using a constraint.

This would break total backwards compatibility with sasview 4.x, but the loader could at least provide a calculation of I(Q) that works, even if to repeat a fit would need a constraint adding.

HOWEVER - the hollow shell models would remain problematic as the user would have to calculate independently the appropriate volfraction_effective. This is where we need a way to send back derived information from the model to the gui.

(Aside - My FISH code had a list of values which were not fittable but could either send or receive information from the models, they could be molar volumes., waters per head group, to use in the model, shell to core or shell to total volume ratio averaged over polydispersity, etc FISH also had "do nothing" parameters such as sld's of sub-components or local solvent fractions which could be used in constraints, and were fittable )

Re constraints - I am going to add a new issue for them!

@pkienzle
Copy link
Contributor

pkienzle commented Apr 4, 2019

How many users need to tie S.volfraction to P.volfraction through <V_enclosed>/<V_shell> for polydisperse systems?

If we remove this feature, then users who need it will have to write a specialized model to calculate their particular P with their own polydispersity calculation, then calculate their particular S and multiply them. So some copy-pasting of code and a fairly simple for loop. Not too difficult, especially if we provide a simple tutorial. This is better than making the whole program more complicated when it is only needed by one or two users.

Maybe fit using a filled model rather than a hollow model in this case? Then the constraint system can tie the parameters together. But interpretation of P.volfraction will require some extra work to scale it to volume fraction of material.

@pkienzle
Copy link
Contributor

pkienzle commented Apr 4, 2019

Are there any refs where people fit vesicles with structure factors? What did they use for the effective volume fraction?

@RichardHeenan
Copy link
Contributor Author

Its perhaps easier to only let users have scale for the external volume fraction of all particles, which is the number they need for volfraction_effective, then have the polydispersity code feed back what the relative values are for the volumes of any enclosed core and shell(s), which is a one-off integration on the parameters for the converged fit. (i.e. scrap vesicle and hollow_cylinder, then use core_shell_sphere and core_shell_cylinder instead, but provide the necessary information)

@butlerpd
Copy link
Member

butlerpd commented Apr 6, 2019

I'm not sure I agree that this is easiest for the user ... assuming I understand what you are suggesting. The point of the vesicle model for example is that the volume fraction users will give is that of the surfactant -- i.e. the only scientifically relevant number they think of.

The problem of course is that, unlike the core-shell sphere, that is completely wrong it when comes to interactions ... which brings me to Paul's question above, most users never worry about interactions because they think they have a small volume fraction and don't pay attention to the fact that it depends entirely on the radius of a vesicle, but typically for 100 nm diameter lipid vesicle the "interaction" vol fraction of spheres is roughly 4 times as I recall the actual vol fraction? so that a 3% lipid with no charge still has a real volume fraction of interacting spheres on the order of 10% at which point even hard sphere interactions should be not insignificant.

So I would argue that

  1. we should always use phi of the actual scientific problem.
  2. while we may want to limit the number of redundant models in our distribution we are moving towards trying to build an easy user reparameterization of those models ... which must allow for things like a vesicle model where phi is surfactant volume fraction while making the default effective radius and/or vol fraction for interactions be the "outer-size"

@butlerpd
Copy link
Member

butlerpd commented Apr 7, 2019

Ok I wrote too fast ... so a couple of new comments as thought of on the plane to Geneva:

  1. Actually the derivation of most models give np * Vp2 where np is the number particle density. This however has zero experimental value. On the other hand, np * Vp is the volume fraction which is a usually a known experimental quantity. More often it is the mass fraction which is known but can fairly readily be converted to volume fraction in most cases. For this reason many people reparameterize the models to replace np * Vp2 with phi * Vp and this has been the approach used by sasmodels. Thus making phi for a vesicle be the vol fraction of the whole sphere completely negates the point of the reparameterization
  2. Thinking about this more deeply, scale, background and resolution are all vestiges of "improper" reduction and thus should not really be part of the models at all (though one could equivocate on background I guess if it is 100.000% due to incoherent scattering from the sample) . In this sense, @pkienzle approach of pulling scale and background out of the models and handling it in the "background" (apologies for the bad pun) is appropriate. For example it allows one to only have one scale and background from a mixture model. Thus I would argue strongly I think for keeping this separation: Models are written assuming a perfectly reduced data set meaning that scale, background and resolution are all handled outside the model definition.
  3. This would then argue again that phi should be a parameter for any model for which it is a parameter. Going back to a theme of @RichardHeenan scale should not take on different meanings depending on the model (= phi IF the model is a particle AND IF it is on an absolute scale but = scale in all other cases).
  4. The rest becomes a UI question of how to present to the user for which there are many choices. For a GUI one could imagine that a toggle check box is provided indicating whether or not one is on an absolute scale (which would then set scale to 1 and hide it from the user if on absolute scale for example). One could also envisage the NXcanSAS format specifying if the data is on absolute scale obviating the need for the user to worry etc. For those using scripting they should presumably have access to the full range of variable and decide what to do with it?

So I guess from the point of view of sasmodels rather than this sasview issue I would propose that:

I(Q) = scale * P(Q) * S(Q). For '''particulate''' models only, where vol_fraction is an experimentally meaningful quantity, P(Q) = P(Q,vol_frac) and the corresponding structure factor would be S(Q,inter_vol_fraction) all defined exactly as they are in the model equations, i.e. Φ * Vp * Δρ2 which multiplies scale, while the interaction vol_frac of S(Q) only affects the shape ... and can be hidden by the GUI if the interaction volume fraction is being set equal to the measured vol_fraction of material.

@smk78
Copy link
Contributor

smk78 commented Apr 8, 2019

@pkienzle asked:

Are there any refs where people fit vesicles with structure factors? What did they use for the effective volume fraction?

There is this "classic": "Light Scattering and Neutron Scattering from Concentrated Dispersions of Small Unilamellar Vesicles", Faraday Discuss. Chem. Soc., 1983, 76, 77-92

@smk78
Copy link
Contributor

smk78 commented Apr 8, 2019

My comment on this debate would be that our Users are confused right now. The question most often asked about SasView is undoubtedly "what does scale mean?". To which some of us might be tempted to respond RTFM, but the fact that we have had to liberally labour our documentation on this point is testament to the seriousness of the issue: we are (apparently) not intuitive on this point. So if we are going to make a change, let's make it intuitive!

@ajj
Copy link
Member

ajj commented Apr 8, 2019

I agree with @butlerpd. Scale is always arbitrary scaling factor. Model contains only parameters needed to calculate scattering on absolute scale.

@butlerpd
Copy link
Member

butlerpd commented Apr 8, 2019

I agree completely with @smk78. The long standing implementation of scale has been a constant source of confusion - largely for the reason that @RichardHeenan suggests: its meaning is constantly shifting. it is called scale which is exactly what it means ... sometimes (i.e. non particulate models). In other cases it means "volume fraction" (particulate models).. oh... but even then it can in fact just mean "scale" (if not on absolute scale). However if we are going to make such a significant change to the infrastructure we do need to think hard about improving the situation not just making a differently confusing solution (with the added benefit that those long time users who know understand it get confused again :-)

I think what I and @ajj are suggesting is that we bite the bullet and call things by the name they are: so vol fraction is vol fraction and scale is scale (we could even call it arb_scale to make it even more clear?).

@pkienzle
Copy link
Contributor

pkienzle commented Apr 9, 2019

Speaking of user confusion, volfraction_shell for vesicle might be better than volfraction.

@ajj
Copy link
Member

ajj commented Apr 10, 2019

Agree with @pkienzle on volfraction_shell - in general we should use more rather than less descriptive names for model parameters

@smk78
Copy link
Contributor

smk78 commented Apr 11, 2019

So what we seem to be coalescing around here is a change that will presumably break backward compatibility? In which case I presume that we would only do it in 5.x? In which case is that 5.0 or 5.1? 5.0 would be the obvious one, but is there time to implement such a change, and test it? And then what do we do about 4.3 which is slated to have the beta approximation?

Some of us agreed at code camp that 4.3 should not have the beta approx due to the effort required to modify the gui, that beta approx should be a selling point for 5.0 Personally I (@RichardHeenan) think that we should get 5.0 "right" before releasing it, even if that takes a few more months.

@RichardHeenan
Copy link
Contributor Author

To summarise I think that we are all agreed that having "scale" stay the same value when introducing an S(Q) is a good idea.

Having a flag on "this data is in absolute units" is not going to work as yes data may be reduced to cm^-1 units but on a sample by sample basis users may not always accurately know either their volume fractions or their sld's.

(i) So, somewhat reluctantly, I think that all particle models do need separate scale and volfraction.

(ii) Controls are then needed in gui to allow only one of volfraction or scale to fit (though beware they might be constrained, so have their check box "on").

(iii) We then need controls on volfraction_effective in any S(Q), this could be say:

(A) as suggested above, on the main fit page a third drop down appears with choices for volfraction_effective_mode along with the ones for structure_factor_mode and radius_effective_mode

[Aside - if we think that these drop downs shiould appear within S(Q) and not at the top with scale & background, now is the time to move them!]

(B) I am worried that (A) will confuse the users even more, but since volfraction_effective normally involves a "simple constraint" we could on first choosing an S(Q) set a default constraint that says M1.volfraction_effective = M1.volfraction which users have the option to disable or remove if they do not want it. This would get users actually looking for and using the constraints.

[ Aside @rozyczko has done a good job on the constraints in 5.0, which were discussed at length with myself, Please try out constraints in 5.0, then leave comments here:
https://github.com/SasView/sasview/issues/1176#issuecomment-479889013 ]

(iv) the special, shell volume fraction models vesicle & hollow_cylinder need extra treatment. I think that these models should also have a new type of "informational" parameter. which cannot be fit but returns e.g. total_to_shell_volume_ratio (averaged over polydispersity). Then in (B) we can by default set the constraint as M1.volfraction_effective = M1.volfraction*total_to_shell_volume_ratio

I realise that the practical implemention of the above is likely non-trivial, but we need to get this right, and I always say that software should serve the scientist, not the reverse.

Whatever we do, this is going to cause some confusion, so a big ? button next to "scale" would be good!

@butlerpd
Copy link
Member

butlerpd commented Apr 12, 2019

So I think the most important thing is to "get it right" before releasing. In principle something like this should be done at a major release version. However as we will discuss some on Tuesday, 5.0 has a really much harder release date than we've done in the past when we have allowed ourselves to keep waiting, sometimes for an extended and mostly "indefinite" period. With that in mind @ajj and I have discussed the fact that we probably need to back off our ambitions on what goes into the immediate release.


As a separate but related question brought up by @smk78 we also need to carefully manage the beta approximation release not only for "getting it right" but now because we are essentially introducing a whole new interface. It is a very delicate balance one needs to manage when trying to make this kind of a change and many don't get it right ... and we may not either but at least we need to go in eyes wide open.

Proper management of the transition suggests indeed that absolutely zero work done on the GUI except for bug management (or to manage new computational features breaking the GUI or just making it too confusing etc etc. Caveats abound). Moreover there must be a deliberate approach to providing incentives to using the new version which will likely be a little more buggy in the beginning as it never gets better without usage ... but at the same time we cannot have an abrupt transition to what was "just working for me" to "not so much anymore" which will always be the case for at least some users even in the best of scenarios.

@RichardHeenan
Copy link
Contributor Author

RichardHeenan commented Apr 12, 2019

For a quicker release of 5.0 I would still like to see the value of scale remaining constant on including an S(Q), as a prelude to later changes to sort out volfraction_effective. The automatic constraint of volfraction_effective in S(Q) would not be there as in 4.2 (but well hidden), but at least any 4.x P(Q)S(Q) model loaded in, could after setting scale to scale*volfraction_effective give a correct forwards calculation of I(Q).

edit - Also in 5.0 suggest rename all volfraction in S(Q) to volfraction_effective

The beta approx and radius_effective are working well in 5.0.

[ There is a specific issue for vesicle, due to name collision with volfraction, which needs some work to the sasview gui - or the volfraction name change to remove the immediate problem - https://github.com/SasView/sasview/issues/1280#issue-427261628 ]

@prjemian
Copy link

Seems the debate here centers on whether or not to delay release of 5.0 (which @butlerpd says has a "much harder release date") so that a "change that will presumably break backward compatibility" can be introduced. Implementation of that change could change the planned release date of 5.0. If it is truly a change that will presumably break backward compatibility, then that change would come in version 6.0 and not 5.1.

Better to keep to the plans (which include a more firm release date for 5.0). If the change added after that is truly API-breaking, then bump the major version to 6.0 with the change.

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

6 participants