-
Notifications
You must be signed in to change notification settings - Fork 30
Adding a pure python truncated octahedron model with Fibonacci orientation averaging #694
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
base: master
Are you sure you want to change the base?
Conversation
I did the test again and it was linked to the starting q value i was using when i created the curve
modification of the two first comment lines
updated references (two more about Fibonacci quadrature)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having different versions with different integration schemes is potentially confusing. If one is clearly better than the other we should be using that.
Controlling the accuracy of the model with a model parameter is unusual. This should never be a fitting parameter. If we want to support this then we need a new parameter type ("constant" or "fixed") to make sure that it doesn't show up as fittable in the GUI.
If fibonacci integration works better than 2D gaussian integration on the sphere then we should be applying it to all our shapes without rotational symmetry (see #658).
Support for Fq and beta is not yet available for pure python models (see #570).
Support for Iqac and Iqabc (orientation and jitter applied before calling kernel) is not yet implemented for pure python models, nor is support for magnetism.
My preference would be to leave this in the model marketplace until sasmodels has complete support for python models, or move the implementation of fibonacci integration to the C model.
I'm concerned about the singularities in the function. The usual trick of switching to a Taylor expansion around the singularity won't work here because the calculation is spread across the polyhedron vertices. Maybe a scheme which collects the bad edges and triangles for later processing would work, substituting zero instead of ≈1e18 in the summation, and correcting the integral for singularities when done. This applies both to the C and the python models regardless of integration scheme.
| denom1 = (qny * qny - qnz * qnz) * (qny * qny - qnx * qnx) | ||
| denom2 = (qnz * qnz - qnx * qnx) * (qnz * qnz - qny * qny) | ||
| denom1 += eps_den * (np.abs(denom1) < eps_den) # add epsilon where denom is zero | ||
| denom2 += eps_den * (np.abs(denom2) < eps_den) # add epsilon where denom is zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting denom to 1e-18 will lead to terms on the order of 1e18. Even when these eventually cancel they will effectively erase the rest of the integral during summation, leading to random values.
You can see if this is a problem using fibonacci lattice point k, setting c2a_ratio such that qny = qnz. Solving, c2a_ratio = b2a_ratio*qb/qc = b2a_ratio*q_unit[k, 1]/q_unit[k, 2].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could remove this eps_den method ?
And make sure that for all Fibonacci points qny=qnz never occurs for any rational value of c2a_ratio ? Taking advantage that gold number is irrational ?
Not sure about this, but it could be tested.
| # build qx,qy,qz arrays with correct broadcasting -> shape (nq, npoints) | ||
| qx = q[:, None] * q_unit[None, :, 0] | ||
| qy = q[:, None] * q_unit[None, :, 1] | ||
| qz = q[:, None] * q_unit[None, :, 2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory required is 3 * 8 bytes * len(q) * npoints_fibonacci, which is 120 MB for 5000 fibonacci points and 1000 q points. Probably okay.
|
|
||
| intensity_grid = np.abs(amp_grid) ** 2 | ||
|
|
||
| integral = np.sum(intensity_grid * w[None, :], axis=1) # summation over all points |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make an Fq version using f1, f2 = sum(amplitude), sum(intensity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but usually, the orientation average over the amplitudes won't be used.
But we can calculate it just in case.
| # implementation). Use 0.0 to avoid producing huge/NaN intensities. | ||
| amp_grid = np.nan_to_num(amp_grid, nan=0.0, posinf=0.0, neginf=0.0) | ||
| # Also clip extremely large values to avoid overflow in square | ||
| amp_grid = np.clip(amp_grid, -1e12, 1e12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This partially address the eps=1e-18 problem above by turning it into eps=1e-12. I'm expecting that you will have 5 or 6 bits of precision in your answer if you happen to hit a singularity.
|
Thank you for all the suggestions. :)
Yes, sure.
Yes, it is a good suggestion to have this possibility to add a new parameter type to avoid 'fiting' with it.
Yes, this is the idea, and it is why we are comparing the two models for truncated octahedron.
Would be great to have Fqabc in pure python model as well. What I have in mind here is a core/shell model for two polyhedron shapes. It is something we do have a lot using seed-mediated growth.
These two options are fine with us. A last idea: maybe usage of numba package could help decreasing the calculation time for pure python even more ?
Yes, we are aware of this issue. It is present for any polyhedron shape, and there are some solutions in the litterature to treat them (but not implemented here, it was too complicated for us to do that until now). Best wishes, |
Yes, we could use numba and/or torch for models, but they use the pure python interface. Oriented 2D, magnetism and model reparameterization are only available for C models. Python models do support polydispersity by looping a parameter like radius over (radius ± Δradius), but we can't do this for orientational dispersion. At the pole (θ=0°) the scattering from φ+Δφ is the same as φ, so ranging over φ±Δφ would have no effect. Instead we first apply dispersion to the shape, rotating the (a, b, c) axes by (Δθ, Δφ, Δψ) [pitch-yaw-roll] then we rotate by (θ, φ, ψ) to set the orientation. We then call Iqabc with the equivalent q vector for the particle in canonical orientation, with the c-axis aligned along the beam. The code for this is in kernel_iq.c for C models, but not yet in kernelpy.py for python models. |
Thank you, that's great that you are mentioning here oriented 2D images. Could we discuss this in a separate location in the github ? |
|
I added ticket #695 for orientation support with pure python models. Orientation dispersion is described in the docs here Note that the gaussian distribution only supports low dispersion levels. For large Δθ, Δφ, Δψ you need to switch to a cyclic gaussian, or the equivalent Maier-Saupe distribution. These are not available in the standard distributions, but are provided as examples of plugin distributions in sasmodels repository. |
Description
Adding a pure python model for truncated octahedrons with orientation averaging using Fibonacci quadrature.
Fixes #686
How Has This Been Tested?
Unit tests worked.
This model was compared with the model using C code (octahedron_truncated.c) and shows good agreement.
In general, the octahedron model was compared to an another numerical calculation : for truncated octahedrons with different sizes and truncatures, agreement was found with the Debye equation (Debye calculator: https://github.com/FrederikLizakJohansen/DebyeCalculator). The Debye equation is based on atomic positions while SasView model is based on analytical expressions.
More tests should be made on small q. Indeed, like the previous model (octahedron_truncated.c), we encouter issues when it comes to small q (q<10^-6 Angs). More precise mathematical should be used (hospital rule, etc).
Note : the fibonacci quadrature code (sasmodels/quadratures/fibonacci.py) was added to a new repository called "quadratures" because it could be also useful for other models.
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)