Add tetrapod model implementation#705
Conversation
Co-authored-by: IndigoCarmine <79552713+IndigoCarmine@users.noreply.github.com>
…-functionality Copilot checked it using formula of the paper.
… author attribution in tetrapod.py
|
@IndigoCarmine. I notice you have left this as a draft request (not ready for review) with the last commit in late March. Just checking to see if you are waiting for or needing something? No worries if you just ran out of time. we all do. Just want to make sure you aren't abandoned. |
|
Sorry for the long silence. My workload has increased recently, and I haven't had much time to work on this. Most of the implementation is already done, but progress has been much slower than I had hoped. I apologize for the delay. I have not abandoned this work and do intend to finish it. However, I cannot promise a timeline at the moment. If someone else is interested in working on it in the meantime, I would be happy for them to build on my current work or even start from scratch. Thank you for checking in and for your patience. |
|
I have finished implementing the standard model (not the core-shell version). The calculated values appear to be consistent with those reported in the reference paper. To be honest, I am not entirely confident about the scaling, so there may still be some issues. |
|
Implemented an initial tetrapod core-shell model. The scattering calculation, documentation, and example results are available and produce reasonable output. The following features are not yet implemented:
|
pkienzle
left a comment
There was a problem hiding this comment.
Code looks okay. Math and plots match those in the paper.
$ PYTHONPATH=. python -m sasmodels.compare tetrapod -q=1e-3:10 -nq=400 background=0 radius=5,10 thickness=0 length=100 scale=100,25
$ PYTHONPATH=. python -m sasmodels.compare tetrapod -q=1e-3:10 -nq=400 background=0 radius=5 length=10,100 scale=100,10
| References | ||
| ---------- | ||
|
|
||
| #. Seoki Kyoo Seo *Korean J. Chem. Eng.* 34(2017) 1192-1198 |
There was a problem hiding this comment.
DOI:10.1007/s11814-016-0341-x
| # [ "name", "units", default, [lower, upper], "type", "description"], | ||
| parameters = [ | ||
| ["length", "Ang", 1000, [0, inf], "volume", "Arm length"], | ||
| ["radius", "Ang", 50, [0, inf], "volume", "Arm outer radius"], |
There was a problem hiding this comment.
In core-shell models this is the core radius rather than the outer radius. See "thickness" in #305
| @@ -0,0 +1,75 @@ | |||
| // acos(-1.0/3.0)/2.0 = half of tetrahedral angle ~54.7356 deg | |||
| #define TETRAHEDRAL_HALF_ANGLE acos(-1.0 / 3.0) / 2.0 | |||
There was a problem hiding this comment.
The following can be precomputed:
cos_A = cos(TETRAHEDRAL_HALF_ANGLE) = √(1/3)
sin_A = sin(TETRAHEDRAL_HALF_ANGLE) = √(2/3)
| parameters = [ | ||
| ["length", "Ang", 1000, [0, inf], "volume", "Arm length"], | ||
| ["radius", "Ang", 50, [0, inf], "volume", "Arm outer radius"], | ||
| ["core_radius", "Ang", 50, [0, inf], "volume", "Arm core radius"], |
There was a problem hiding this comment.
You are still subtracting thickness from radius internally, so this parameter is outer radius, not the core radius. To avoid confusion with core-shell models, I suggest changing the name to radius_outer or leaving it as radius and have thickness add to the radius rather than subtract from the radius.
We are following the "mathematical" convention radius_tag rather than the natural language tag_radius for most parameter names in our models.
|
@IndigoCarmine - @marimperorclerc has now done the functionality review which she posted above in thread about overestimation of the volume so pointing it out here to make sure you see it. She has only two comments, one of which @pkienzle already suggested; mainly that the limitations of the model (length vs diameter etc) should be clearly stated in the documentation text. The other is the standard request of having an image in that documentation to help users understand what the model is for. Once those are addressed, both code and functionality reviews will be complete I think and we can merge this. Sorry it took so long. |
|
Thanks for the reviews, @marimperorclerc, @pkienzle, and @butlerpd. The two remaining comments have now been addressed:
The earlier comments have also been addressed:
The documentation builds cleanly, and all model tests pass. Please let me know if anything else needs to be addressed. |
|
I would like to express my sincere gratitude to everyone who contributed to this pull request! |
This pull request introduces a new tetrapod-shaped particle model.
#641
The current implementation and accompanying documentation include some AI-generated context that has not yet been thoroughly reviewed or validated. Sorry. I will revise it soon.
The issue originally proposed a CoreShell model, but for the moment I will implement the model as described in the referenced paper.
Reference:
https://github.com/user-attachments/files/19167378/s11814-016-0341-x.pdf