Skip to content

Conversation

@orionarcher
Copy link
Collaborator

@orionarcher orionarcher commented Apr 8, 2025

Summary

Include a summary of major changes in bullet points:

Checklist

Work-in-progress pull requests are encouraged, but please enable the draft status on your PR.

Before a pull request can be merged, the following items must be checked:

  • Doc strings have been added in the Numpy docstring format.
    Run ruff on your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

@cla-bot cla-bot bot added the cla-signed Contributor license agreement signed label Apr 8, 2025
@orionarcher orionarcher added enhancement New feature or request fix Bug fix labels Apr 8, 2025
@abhijeetgangan
Copy link
Collaborator

LGTM. Nice use of scatter operation, btw :)

@abhijeetgangan
Copy link
Collaborator

Something seems wrong in the graph pes test- https://github.com/Radical-AI/torch-sim/actions/runs/14406977027/job/40405760127?pr=123

Unrelated to this PR.

@orionarcher
Copy link
Collaborator Author

orionarcher commented Apr 11, 2025

@jla-gardner @CompRhys, seems like one of the graph-pes tests fails intermittently, I am inclined to just loosen the bound to 5e-5?

@CompRhys
Copy link
Member

CompRhys commented Apr 11, 2025

@jla-gardner @CompRhys, seems like one of the graph-pes tests fails intermittently, I am inclined to just loosen the bound to 5e-5?

I realize that I didn't implement what I thought I had for the rattle, I was planning to do closer to

    perturbed = struct.copy()
    for site in perturbed:
        magnitude = rng.weibull(gamma)
        vec = rng.normal(3)  # TODO maybe make func recursive to deal with 0-vector
        vec /= np.linalg.norm(vec)  # unit vector
        site.coords += vec * magnitude
        site.to_unit_cell(in_place=True)

based on CGCNN+P from MBD but didn't as I didn't sample the directions. We should just replace with a gaussian rattle will be easier.

--edit

I fixed this in 1344660.
The Weibull gives a much nicer distribution for rattling so I stuck with it rather than going Gaussian.

@orionarcher orionarcher enabled auto-merge (squash) April 12, 2025 20:57
@orionarcher orionarcher merged commit 60e80b3 into main Apr 12, 2025
85 checks passed
@orionarcher orionarcher deleted the support_different_batch_temps branch April 12, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed Contributor license agreement signed enhancement New feature or request fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants