Opened 3 years ago

Closed 3 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 3 years ago by pkienzle

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.

comment:2 Changed 3 years ago by smk78

  • Owner set to pkienzle
  • Status changed from new to assigned

comment:3 Changed 3 years ago by smk78

Will standardise all models to standard rounding.

comment:4 Changed 3 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 3 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: Changed 3 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 3 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 3 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
Last edited 3 years ago by pkienzle (previous) (diff)

comment:9 Changed 3 years ago by pkienzle

star polymer question moved to ticket #867

comment:10 in reply to: ↑ 6 Changed 3 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 3 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 3 years ago by GitHub <noreply@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 9ae85f0f8fc9f144ef76a1134a4aac6439652990/sasmodels:

Merge pull request #23 from SasView?/ticket-815

Use nearest integer for discrete parameters. Closes #815

Note: See TracTickets for help on using tickets.