Opened 4 years ago
Closed 4 years ago
#815 closed defect (fixed)
check that integer parameters are handled properly
Reported by: | pkienzle | Owned by: | pkienzle |
---|---|---|---|
Priority: | major | Milestone: | SasView 4.1.0 |
Component: | SasView | Keywords: | |
Cc: | Work Package: | SasView Bug Fixing |
Description
sasmodels and bumps do not currently support integer parameters. They are instead sent to the model as floating point values, which may be non-integers when fitting.
For each model that has integer parameters:
- check that the parameters is converted to an integer,
- check that the conversion is consistent in Iq, Iqxy, form_volume, ER, VR, and profile,
- check that 0 is supported or excluded via an INVALID macro,
- check that conversion from float to conversion is consistent,
Should probably be consistent in choice of truncation or rounding. Currently some models use rounding and others use truncation, so sasview management will need to decide which is preferred. The choice should be documented in the plugin models documentation.
Rounding has a slight advantage as far as bumps uncertainty analysis is concerned since the average of [n-0.5, n+0.5] is n. Also, it may be slightly more natural for the user to see a value of 3.9 as 4 rather than 3, as would happen for truncation using trunc or floor. If using rounding, the rint function is slightly better than round in that it removes the bias by having n+0.5 sometimes round up and sometimes round down, depending on n; neither round nor rint have been used, so check whether they exist on all C compilers and OpenCL platforms before using.
Specific models:
- pearl_necklace uses floor(n+0.5), which is round. linear_pearls should use the same as pearl_necklace.
- stacked_disks uses a loop which is equivalent to ceil, but the normalization by n_stacking does not move to the equivalent integer.
- polymer_micelle leaves n_aggreg as a float. Make sure the equations make sense for real values.
- multilayer_vesicle uses a loop which is equivalent to ceil.
- lamellar_stack_caille, lamellar_stack_paracrystal, and lamellar_hg_stack_caille use trunc.
- spherical_sld uses a loop which is equivalent to ceil for n_steps. Note: n_steps should not be a fittable parameter.
- core_multi_shell, onion, spherical_sld do not use a fittable value for the number of shells so it doesn't matter for now. Similarly for unified_power_Rg and rpa.
Change History (12)
comment:1 Changed 4 years ago by pkienzle
comment:2 Changed 4 years ago by smk78
- Owner set to pkienzle
- Status changed from new to assigned
comment:3 Changed 4 years ago by smk78
Will standardise all models to standard rounding.
comment:4 Changed 4 years ago by pkienzle
Raspberry model estimates the number of small particles from the volume fractions:
//Number of small particles per large particle Np = vfS*fSs*VL/vfL/VS;
I'm leaving this as a floating point value rather than rounding to the nearest integer.
Note: volume calculation in raspberry is done in Iq(), and so the polydispersity average will not be volume weighted.
comment:5 Changed 4 years ago by pkienzle
The star polymer model doesn't seem to require an integer number of arms, so I'm leaving the arms parameter as a floating point number.
If arms is indeed an integer, then the parameter should be renamed to n_arms.
comment:6 follow-up: ↓ 10 Changed 4 years ago by smk78
Looking at Higgins & Benoit (as I don't have access to the reference given for the star model) I suspect that arm is a relatively modern term and that branch was the original terminology.
That being so, I find it difficult to comprehend how you can have a non-integer number of branches.
The branches may be of different length (cf molecular weight, degree of polymerisation), but that is a different issue.
So I would vote that we make arms into n_arms.
comment:7 Changed 4 years ago by ajj
Comments ported from github:
In general, models where the math works with non-integer values should not force the value to an integer. The user must decide how to deal with it and the documentation should make clear that it should be integer.
Other lamellar models could probably be modified to handle non-integer numbers of layers with a weighted sum. Docs would have to clearly indicate that this was being done.
Raspberry model should not enforce integer numbers of rasps, nor have the number of rasps as a parameter. Tried this previously and it didn't work - model became mathematically unstable.
If we had a mechanism for models returning derived values (values calculated from the fit parameters) then number of rasps would be one in this case.
comment:8 Changed 4 years ago by pkienzle
The changes are mostly to make sure that float to int conversion is consistent across models (I use rounding) and that form volume uses integer if the form function uses integer. This will result in a number of models producing different results from before for non-integer inputs. Models which supported non-integer inputs in 3.x will continue to support non-integer inputs.
Here are the models affected by this patch:
linear_pearls: implicit int via for loop multilayer_vesicle: implicit int via for loop stacked_disks: implicit int via for loop core_multi_shell: was forcing int via ciel lamellar_hg_stack_caille: was forcing int via trunc lamellar_stack_caille: was forcing int via trunc pearl_necklace: was forcing int via round onion: int via multiplicity rpa: int via multiplicity spherical_sld: int via multiplicity unified_power_Rg: int via multiplicity lamellar_stack_paracrystal: correcting for non-integer portion polymer_micelle: not forcing int raspberry: not forcing int star_polymer: not forcing int flexible_cylinder: code cleanup; shouldn't be part of this ticket
comment:9 Changed 4 years ago by pkienzle
star polymer question moved to ticket #867
comment:10 in reply to: ↑ 6 Changed 4 years ago by butler
actually the term is not that new and in principle differs from brached, as the name might imply, in that all the branches originate from a central point. A comb polymer on the other hand has lots of branches coming of the backbone in a "comblike fashion" etc. You are correct that one usually refers to n_star (4 arm star, 6 arm star etc).
Replying to smk78:
Looking at Higgins & Benoit (as I don't have access to the reference given for the star model) I suspect that arm is a relatively modern term and that branch was the original terminology.
That being so, I find it difficult to comprehend how you can have a non-integer number of branches.
The branches may be of different length (cf molecular weight, degree of polymerisation), but that is a different issue.
So I would vote that we make arms into n_arms.
comment:11 Changed 4 years ago by richardh
In a comment to #843 multilayer vesicle, Paul K said
(1) Re integer rounding, a number of different models are now rounding to N. This should be noted in those models, but a general discussion of the effect this has on fitting is not model specific. That is, don't use derivative based models if you are fitting N.
(2) Re n=1, Tw is ignored so will allow arbitrary values. Bumps should return this as an extremely large uncertainty; other fitters should say that the system is singular and they can't compute uncertainty. Is this so bad? (Note: I haven't tested fitting simulated data with N=1, but that is what it should do.)
(3) Re epsilon wave, not sure what you mean. Are you talking about the epsilon ball radius used in DREAM fitting? A longer burn-in time will allow DREAM to navigate between steps.
We (OK Richard) ought to look at this, possibly at code camp. Most users will blindly use Levenberg-Marquardt least squares, so may expect integer N parameters to adjust nicely. So may be better to interpolate a mix of N and N+1 models, even though that might double the calculation time. In some of the summing of a series models, like multilayer_vesicle, some simple modifications of the code could adjust the final terms for non-integer N.
Richard
comment:12 Changed 4 years ago by GitHub <noreply@…>
- Resolution set to fixed
- Status changed from assigned to closed
Note: polydispersity on integer parameters will be better behaved using rounding rather than truncating, at least while we are using the existing continuous distributions for discrete polydispersity.