SasView / sasmodels

Package for calculation of small angle scattering models using OpenCL.
BSD 3-Clause "New" or "Revised" License
16 stars 28 forks source link

Ticket 1265 superball #478

Closed dehoni closed 3 years ago

dehoni commented 3 years ago

The superball model allows to quantify if a nanoparticle is cuboidal or spherical with a parameter describing the sharpness of corners and edges. The superball form factor allows to differentiate between effects arising due to particle size distribution and the irregular particle shape.

pkienzle commented 3 years ago

Math corresponds to code. Analytic function matches MC simulation (I updated realspace to include Superball). Limiting cases produce correct form volume.

I don't know why F is not scaled by form_volume in calculating Fq and Iqabc. Is volume somehow incorporated in the leading length_a squared term?

Regardless, I was unable to match a polydisperse sphere with a polydisperse superball using:

sascomp -highq -single! -nq=100 superball,sphere length_a=100 length_a_pd=0.2 radius=length_a/2 radius_pd=length_a_pd/2 exponent_p=1

Using the volume-weighted Fq in the sphere model, the polydispersity distribution is on number density, and this may be the source of the difference.

Doc strings could use \sin, etc. instead of \mathrm{sin}.

Could use a u=cos(θ) substitution when computing the orientation average, as is done for other shapes. I'm not sure if there is a significant advantage one way or the other.

dehoni commented 3 years ago

I don't know why F is not scaled by form_volume in calculating Fq and Iqabc. Is volume somehow incorporated in the leading length_a squared term?

Regardless, I was unable to match a polydisperse sphere with a polydisperse superball using:

sascomp -highq -single! -nq=100 superball,sphere length_a=100 length_a_pd=0.2 radius=length_a/2 radius_pd=length_a_pd/2 exponent_p=1

Using the volume-weighted Fq in the sphere model, the polydispersity distribution is on number density, and this may be the source of the difference.

The group has been contacted to have a look into this issues.

DomiDre commented 3 years ago

Hello hope ya'll doing fine, great to hear we are thinking about incorporating the code :smile:

I don't know why F is not scaled by form_volume in calculating Fq and Iqabc. Is volume somehow incorporated in the leading length_a squared term?

Actually the volume is included in the numerical integral calculated in the oriented_superball function. Like when you integrate for the sphere volume over exp(i q.r) in spherical coordinates and it pops out the 4pi/3 R^3 magically - which one then normally divides by so only the sas_3j1x_x remains... The numerically calculated formfactor amplitude of the superball is at no point divided by the volume, as it would have needed the call for two gamma functions, which I thought one could save for calc. speed up, if only the formfactor amplitude weighted by the volume is considered anyway? The 2a^2 correspond to the 8r^2 prefactor of the integral in the corresponding formula in the paper.

Regardless, I was unable to match a polydisperse sphere with a polydisperse superball using: ...

I tested the command but used radius_pd=length_a_pd instead of radius_pd=length_a_pd/2, then the two form factors match except for high q - values above q=0.65. I think pd is relative and therefore 20% should be the same for a and r, or?

Could use a u=cos(θ) substitution when computing the orientation average, as is done for other shapes. I'm not sure if there is a significant advantage one way or the other.

Mhh I think that would be a good idea for some optimization. Substituting cos(θ) would remove one calculation of cosine and sine and replace it by u and sqrt(1-u^2). I can do the modification if nobody opposes.

dehoni commented 3 years ago

Hi Dominique,

Nice to see you back doing science (as long as the German IT stays safe so long). The substitution is already done with the second to last commit.

dehoni commented 3 years ago

The remarks of @DomiDre are now also reflected in the model.

DomiDre commented 3 years ago

Hi Dominique,

Nice to see you back doing science (as long as the German IT stays safe so long). The substitution is already done with the second to last commit.

Don't you call any demons on me :smile:. I think what Paul meant, was to replace the call of SINCOS by the substitution in the theta integral. I've done it, but sadly don't see any significant speedup to mention. But I guess it looks nicer this way.

pkienzle commented 3 years ago

@DomiDre, you are correct about the pd. It is relative and doesn't need to be scaled.

I verified that the limitation to q < 0.65 is due to under sampling in the integrals. You can see this by increasing the number of points in the integral to 76, which (eventually) shows a difference at q=4. First delete ~/.sasmodels/compiled_models/sas64_superball.so then run with with -exq -double -ngauss=76 -nq=50 length_a_pd=0.

I'll put in a ticket to address the speed/accuracy issues.

wpotrzebowski commented 3 years ago

Hmm, do we know why ubuntu test is failing?

dehoni commented 3 years ago

Hmm, do we know why ubuntu test is failing?

The issue is fixed and all checks are green.

wpotrzebowski commented 3 years ago

sasmodels ready for testing on OSX

wpotrzebowski commented 3 years ago

This time unfortuantely Jenkins's complained: https://jenkins.esss.dk/sasview-beta/job/sasmodels-OSX10.13-ghprb/32/console

dehoni commented 3 years ago

sasmodels ready for testing on OSX

pkienzle commented 3 years ago

Umlauts should work.

After merging with master, the failing code is https://github.com/SasView/sasmodels/blob/ab4117caffd4bab6bec0199088005572fddb6bfc/doc/genmodel.py#L293-L294

I've tested tested the following on python 3.6 and 3.8 on my mac without getting an error:

with open('test_uni.txt', 'w') as fid: fid.write("\xfc")

Maybe it would work to set an explicit encoding on open:

with open('test_uni.txt', 'w', encoding='utf-8') as fid: fid.write("\xfc")

You will have to merge master into your branch before trying this because your version of doc/genmodels.py is out of date.

dehoni commented 3 years ago

sasmodels ready for testing on OSX

pkienzle commented 3 years ago

@wpotrzebowski I don't understand why the write code is failing for umlauts. Can you try the open/write calls directly on the Jenkins machine to figure out what is going on?

wpotrzebowski commented 3 years ago

@wpotrzebowski I don't understand why the write code is failing for umlauts. Can you try the open/write calls directly on the Jenkins machine to figure out what is going on?

Hmm, Umlauts seem to work but I am also getting this warning/error: /Users/sasview/Jenkins/workspace/sasmodels-OSX10.13-ghprb/sasmodels/doc/model/superball.rst:117: WARNING: image file not readable: model/img/superball_autogenfig.png

pkienzle commented 3 years ago

I've seen this when the image generation code was interrupted. With caching in genmodel.py https://github.com/SasView/sasmodels/blob/79e9f4a8ccb23fe814c80ea06406d78f90358c27/doc/genmodel.py#L363-L365 even the rebuild all in Makefile https://github.com/SasView/sasmodels/blob/79e9f4a8ccb23fe814c80ea06406d78f90358c27/doc/Makefile#L54-L56 will not attempt to generate the image when the rst file already exists. Either you need to touch superball.py or clear superball.rst from the cache. And maybe check for a zero-length superball png in the image cache.

dehoni commented 3 years ago

sasmodels ready for testing on Win

dehoni commented 3 years ago

sasmodels ready for testing on OSX

dehoni commented 3 years ago

SOLVED: Deleting the categories.json will create updated categories. There is another issue with updating changes in the model files, that I have encountered. The model should appear under the category "sphere", but an own "superball" category is created. I tested also with other models simply changing the category in the model describtion. The behaviour is similar: the model will stay in the old group and does not switch to the new category.

wpotrzebowski commented 3 years ago

ready for testing on OSX

wpotrzebowski commented 3 years ago

sasmodels ready for testing on OSX

wpotrzebowski commented 3 years ago

@pkienzle @dehoni setting Single=False solved the problem with builiding doc but I don't know if this is what we want for this model.

pkienzle commented 3 years ago

Model precision problems are not due to single vs. double. The results compare identically to float32 precision.

$ sascomp superball '-engine=single!,double!'
DLL[32] t=1336.86 ms, intensity=12091
DLL[64] t=1536.00 ms, intensity=12091
|DLL[32]-DLL[64]|            max:3.301e-05  median:8.739e-06  98%:2.488e-05  rms:1.193e-05  zero-offset:+9.863e-06
|(DLL[32]-DLL[64])/DLL[64]|  max:3.391e-07  median:9.261e-08  98%:2.553e-07  rms:1.253e-07  zero-offset:+1.042e-07

I have been having problems on Mac with compilers and/or OpenCL drivers. Does it help to use SAS_OPENCL=None in the environment for OS/X? Or is it failing on other architectures?

wpotrzebowski commented 3 years ago

Model precision problems are not due to single vs. double. The results compare identically to float32 precision.

$ sascomp superball '-engine=single!,double!'
DLL[32] t=1336.86 ms, intensity=12091
DLL[64] t=1536.00 ms, intensity=12091
|DLL[32]-DLL[64]|            max:3.301e-05  median:8.739e-06  98%:2.488e-05  rms:1.193e-05  zero-offset:+9.863e-06
|(DLL[32]-DLL[64])/DLL[64]|  max:3.391e-07  median:9.261e-08  98%:2.553e-07  rms:1.253e-07  zero-offset:+1.042e-07

I have been having problems on Mac with compilers and/or OpenCL drivers. Does it help to use SAS_OPENCL=None in the environment for OS/X? Or is it failing on other architectures?

According to the discussion we had at the developers meeting we move to GH actions builds, so I guess no need for fixing it on Jenkins...

wpotrzebowski commented 3 years ago

@pkienzle just to make sure for this model we shouldn't use single=False? I can revert this change and merge it.

How about this commit: https://github.com/SasView/sasmodels/pull/478/commits/cd69b90e34a1b044e117f04caa0542234cc939ae? Should we keep it?

pkienzle commented 3 years ago

I don't think we need single=False for this model, at least not for numerical reasons. I suspect using it is hiding a problem with opencl for one of the platforms.

For example on my mac I had to remove const from https://github.com/SasView/sasmodels/blob/136e63204bbcf98f579ef781c6fefa63637c6574/sasmodels/kernel_iq.c#L129 because the superball branch had an old version. This might have been what was causing the model not to build properly.

Similarly, regarding the commit I don't know why the file would exist but not be accessible. And if it isn't accessible, then you probably can't overwrite it. So this is solving a problem that shouldn't exist if you are starting with a clean environment.

pkienzle commented 3 years ago

This PR is changing a number of files. I think you need to merge master into this branch and check that the diff looks reasonable before merging back into master.

wpotrzebowski commented 3 years ago

sasmodels ready for testing on OSX

wpotrzebowski commented 3 years ago

@dehoni and @pkienzle I am planning to merge it (after reverting recent commits) even though it is failing on Jenkins (it passes all other tests)

wpotrzebowski commented 3 years ago

sasmodels ready for testing on Win