shape2sas plugin models #3544
shape2sas plugin models #3544
Conversation
klytje
force-pushed
the
shape2sas_fitting
branch
2 times, most recently
from
499f31e to
f69e375
Compare
|
It is not ideal that the automatic |
I'm surprised ruff isn't complaining about the This might be a good opportunity to get rid of the * imports in this new code? (And of course progressively remove them in old code) |
|
|
||
| #Generate subunits | ||
| for i in range(self.Number_of_subunits): | ||
| Npoints = int(self.Npoints * volume[i] / sum_vol) |
There was a problem hiding this comment.
Point density will be lower than expected if there is significant overlap between the subunits.
| @@ -0,0 +1,10 @@ | |||
| from dataclasses import dataclass | |||
There was a problem hiding this comment.
Here and elsewhere.
From PEP 8:
Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.
If you use uppercase letters in the module name you have to be sure that all the imports also use uppercase letters. If you are developing on a file system that doesn't distinguish upper and lower case you won't see the problem, but it may appear on Mac or Linux, depending on how the OS is configured.
There was a problem hiding this comment.
This means I should change the filenames to be lowercase? I can do that, but it's not enforced in other areas of SasView.
| z = np.random.uniform(-self.l / 2, self.l / 2, N) | ||
| d = np.sqrt(x**2 + y**2) | ||
| idx = np.where((d < self.R) & (d > self.r)) | ||
| x_add, y_add, z_add = x[idx], y[idx], z[idx] |
There was a problem hiding this comment.
Rejection is inefficient, especially for narrow-walled pipes. It is better to use:
radius = np.sqrt(np.uniform(self.r**2, self.R**2, N))
... then follow the r == R case above ...
You can also use the gaussian trick discussed in Ellipsoid below. I'm not sure which is faster.
|
|
||
| d2 = x**2 / self.a**2 + y**2 / self.b**2 + z**2 / self.c**2 | ||
| idx = np.where(d2 < 1) | ||
| x_add, y_add, z_add = x[idx], y[idx], z[idx] |
There was a problem hiding this comment.
Instead of rejection you can use transformation. From math exchange here, the unit vector for a randomly generated gaussian will be uniform on the surface of a sphere, so normalize x = rand(n, 3) by sumsq(x) and scale by the cubed root of U(0,1). This gives random points in a sphere. For a triaxial ellipsoid, stretch by (a, b, c).
This gives
v = np.random.randn(Npoints, 3)
r = np.cbrt(np.random.rand(Npoints))
s = v*(r/np.sqrt(np.sum(v**2, axis=1)))[:,None]
x, y, z = s.T * np.array([[a,b,c]]).TYou can do something similar to sample from spherical shells, choosing r = cbrt(U(inner**3, outer**3, Npoints))
| d = np.sqrt(x**2 + y**2 + z**2) | ||
|
|
||
| idx = np.where(d < self.R) #save points inside sphere | ||
| x_add,y_add,z_add = x[idx], y[idx], z[idx] |
There was a problem hiding this comment.
See comment in ellipsoid. Or just use ellipsoid with a=b=c=r.
| z = np.random.uniform(-self.l / 2, self.l / 2, N) | ||
| d = np.sqrt(x**2 + y**2) | ||
| idx = np.where(d < self.R) | ||
| x_add,y_add,z_add = x[idx],y[idx],z[idx] |
There was a problem hiding this comment.
You can use polar coordinates, with r ~ sqrt(U[0, 1]), θ ~ U[0, 2π], or use the gaussian trick discussed in Ellipsoid below for the circular cross section. These should be more efficient then the rejection method.
| try: | ||
| return globals()[key] | ||
| except KeyError: | ||
| raise ValueError(f"Class {key} not found in subunitClasses or global scope") |
There was a problem hiding this comment.
Looking for a user supplied name in the global scope is a bit risky. You might be better off registering new shapes when they are imported, similar to the plugin models.
src/sas/sascalc/shape2sas/Models.py
Outdated
|
|
||
| z_rot = (-self.x_add * np.sin(self.beta) | ||
| + self.y_add * np.cos(self.beta) * np.sin(self.alpha) | ||
| + self.z_add * np.cos(self.beta) * np.cos(self.alpha)) |
There was a problem hiding this comment.
Be sure the application of the Euler angles is consistent with other places in SasView. For my own version shape2sas in the sasmodels explore directory here I'm using z-y-x convention for φ-θ-ψ, which is consistent with the convention I'm using for oriented particles in the sasmodels kernels.
There was a problem hiding this comment.
The conventions differ. Shape2SAS uses the intrinsic ZYX convention, while you seem to be using ZYZ with exchanged angles (see the new test case here).
|
@pkienzle These are good points, but all of them are inherited from the existing shape2sas implementation. I just moved the model definitions out into separate files for clarity. I'll fix the bugs, but the efficiency concerns are perhaps better saved for another PR to keep this one focused on the extension to plugin models? |
Ruff does complain about the In my recent work on reducing the number of linting errors I have removed the vast majority of The cause of the problems here was the use of the We now have an ADR proposal for type hints #3170. In this ADR it is explicitly stated that built-in types should be used instead of the |
Personally I'd disagree with this rule - while wildcard imports from larger modules is obviously a terrible idea, I have here specifically designed the
Again I'd like to point out that this is inherited from the existing shape2sas implementation. |
|
This was discussed at today's fortnightly call. It is noted that this also should be dealt with prior to the camp. It has been quite a while since the last discussion and there were questions as to whether all the issues had been addressed which nobody could remember off hand. Note that @butlerpd has been tagged to review, and as someone tried to get this working before 6.1.0 is probably reasonably positioned to do so. @klytje, it is noted that not fixing all the inherited stuff is not necessarily reasonable at this point. Other than that could you update whether there are any outstanding issues/discussions in this PR as far as you are concerned? |
|
@butlerpd I (attempted to) address all of @pkienzle's comments except for those related to improving the sampling efficiency. I'd rather not touch any of that stuff without an extensive test suite to validate against (#3310), which is out of scope for this PR. The only remaining issue is that of Euler rotation convention, where We also have the issue of user testing, which is what has really been holding up this PR. I've done my best to make it as easy to use and robust as possible, but I need actual user feedback on these aspects. |
klytje
force-pushed
the
shape2sas_fitting
branch
from
b688682 to
e3b89a4
Compare
|
While the plugin model generator contributed in this PR is fully functional, Should users be interested in trying out the plugin models regardless of these issues, it can easily be enabled with the following two commands, to be run from Remember to manually enable the Differential Evolution algorithm in |
…nd of refactoring for now
klytje
force-pushed
the
shape2sas_fitting
branch
from
8199966 to
4b2353d
Compare
Description
This PR adds support for generating and fitting plugin models from the
shape2sasmodel builder.GUI
to plugin modelbutton in the mainshape2saswindow has been re-enabled and fully implemented.Constraints
Constraints are now fully supported.
parametersvariable while trying to preserve user changes to these. I'm not sure how robust the current code is, so this functionality should be extensively tested.Several convenience variables have been added to simplify the user code:
'delta'-variants are defined for all shape parameters and may be used to easily constrain variables.
dR1 = dR2where dR2 is a fit parameter will propagate all changes in dR2 to dR1. Note how this is very different to writingR1 = R2(also a valid constraint) which will fix their values to be identical. Using delta variables allows the initial values to be different while still tracking changes.A barbell (sphere 1, cyl 2, sphere 3) may therefore be constrained as:
dR3 = dR1dL2 = -2*dR1where R1 is a fit parameter.
'COM' may be used to easily link all center-of-mass parameters (
COMX, COMY, COMZ).dCOM2 = dCOM1is automatically expanded todCOMX2, dCOMY2, dCOMZ2 = dCOMX1, dCOMY1, dCOMZ1.Most of the complexity of this PR comes from supporting delta parameters.
Efficiency
AUSAXS, which is significantly faster (I measured 30x). I am not sure how feasible the fitting process is without this change.Edit:libausaxs is not included in installers #3545.AUSAXSis not available in the installers, seeshape2sasfits, likely due to the function landscape variations arising from the limited sample size.How Has This Been Tested?
This will require significant testing before being merged, though I don't have any good data to perform this testing with. The simulated data from
shape2sashas too large error bars to be useful I think.Todo
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)