Repository navigation
Conversation
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
Code looks reasonable, but I do have a couple of comments and questions. I haven't tested, but unit tests in CI pass.
If possible, adding new tests to the tetrahedron and prism models would be nice to have but the values would need validating, so I understand why they aren't included.
| parameters = self._parameter_vector | ||
| n_pars = len(parameters) | ||
| # Skip scale and background | ||
| parameters[:] = values[2:n_pars+2] |
There was a problem hiding this comment.
Are the first two parameters always guaranteed to be scale and background now and into the future?
There was a problem hiding this comment.
They are that way now. If it changes in the future we can update this code.
They parameter vector layout is assumed in a few places within the package, but third party code shouldn't need to know it.
| if (rot != self._rotate[:2]).any(): | ||
| print(rot) | ||
| print(self._rotate[:2]) | ||
| raise Hell |
| }, [0.01, 0.05, 0.2], [87.50350037, 63.95202427, 0.27889988] | ||
| }, | ||
| [0.01, 0.05, 0.2], | ||
| [87.24724230561272, 64.13160420356327, 0.2767562850617265], |
There was a problem hiding this comment.
Why are these test values changing now?
There was a problem hiding this comment.
length_pd_n now defaults to 35 (issue #746 resolved incidentally in this PR because it helped with testing)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…n order of operations
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Add support for Iqac and Iqabc in pure python models. #695
Fixes polydispersity and volume normalization for pure python models.
Defaults
*_pd_nto 35. #746There is still a descrepency between c models and their python equivalent with theta orientational dispersion. If it is not yet clear whether it is problem in the C kernel or the python kernel. If it is not fixed in this PR then add as a separate issue.