Generative rotated molecules from a finite set of particules #2
Conversation
3f4d697 to
8dd5c80
Compare
There was a problem hiding this comment.
Looking good!! Thanks @NicolasLegendre1 . I've left a few comments.
|
|
||
| def get_random_quat(num_pts): | ||
| """Generate a list of quaternions by using uniform distribution. | ||
|
|
There was a problem hiding this comment.
How do you know this samples from a uniform distribution, do you have a reference (Wikipedia, research paper) that explains why these formulas work?
There was a problem hiding this comment.
okay I will add that but i'm not sure how to present that into the Docstring of the function
| volume = np.zeros((vol_size,) * 3) | ||
| volume = sim_volumes.modify_weight(points, volume, vol_size, center) | ||
| assert volume.shape == (vol_size,) * 3 | ||
| assert volume[1][1][1] == 3.4866983294215885e-164 |
There was a problem hiding this comment.
How did you get this value?
If you run sim_volumes.modify_weight on your computer and then copy-pasted it here, it's slightly less effective as a unit-test (it's still useful, as it protects your code from being modified by other contributors).
Is there a simpler test (with a simpler volume) that could be run here, so that the volume[1][1][1] can be computed manually - so that the manual computation is the check for the modify_weight's computation?
|
@NicolasLegendre1 you can ignore the DeepSource errors. @fredericpoitevin could you make deepsource automatically ignore the above errors (see my comment in PR compSPI/ioSPI#14)? There's no hurry though, we can merge the PRs with DeepSource failing. Also, it seems that the DeepSource transformers have been run on this PR, as opposed to PR compSPI/ioSPI#14 ! The difference between this PR and the PR#14 from ioSPI is that PR#14 comes from the branch on Nicolas's fork, and this PR is directly on a branch on the main repo. In order for the DeepSource transformers to run on your PR#14, Nicolas you will need to enable DeepSource on your own ioSPI repo. |
Thanks @NicolasLegendre1 and @ninamiolane! |
|
@fredericpoitevin @NicolasLegendre1 deep sources errors are ignored! Ready to merge once the comments are addressed 🎉 |
30c3714 to
2aac5dc
Compare
There was a problem hiding this comment.
@NicolasLegendre1 @fredericpoitevin LGTM! (I've seen a few other details, but in the interest of merging PRs slightly faster I am OK to merge now 🤗 ). Should we contact Youssef or Julien to officially merge here?
Let's merge this PR now. I'd like to homogenize the structure across compSPI repos before soliciting maintainers: what do you think? |
Merging Arjun's branch into wrapper-function-headers
merge compSPI/simSPI:master into geoffwoollard/simSPI:multisclice_compspifft
No description provided.