-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ruff
fixes
#184
ruff
fixes
#184
Conversation
@janosh Can you please approve the workflow? Thanks! |
Thanks! I believe the test failure is owing to the lack of support of NumPy 2.0 from |
Pymatgen should go first and ignore failing chgnet tests |
Yes I just realized the same, |
@janosh It looks like you have to approve the workflow again for some reason? |
until your first PR to a repo is merged, workflows need to be re-approved on every commit. it's quite annoying |
Hi @janosh if i remember correctly, this cannot be merged until a new release of |
@DanielYang59 thanks for the reminder. pymatgen v2024.9.10 should be on PyPI in a few minutes |
@janosh Have to ping you to approve the workflow (again), hopefully no test fails this time |
@janosh Should be good this time :) |
Concur, we should focus on inferred (implicit) types, i.e. Instead of just changing the type in tests, we should also update the code where However, I'm not familiar with the code base so if @BowenD-UCB could give a hand that would very helpful.
It has not been delayed at all :) |
ab855b6
to
610b641
Compare
da3e51b
to
bc1d485
Compare
… permission for some reason
bc1d485
to
4cafef7
Compare
@janosh I reverted all (I believe) numpy related change from this PR, and relocate NumPy v2 migration to another PR as it turns out there's more debugging needed than I expected. Might need to replace the labels for this PR |
structure_perturbed.perturb(distance=0.1) | ||
fixed_rng = np.random.default_rng(0) | ||
with patch("numpy.random.default_rng", return_value=fixed_rng): | ||
structure_perturbed.perturb(distance=0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably add a seed
kwarg to Structure.perturb
that's simply passed into np.random.default_rng(seed=seed)
over in pymatgen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, especially with the generator implementation.
I didn't realize this downside before but it turns out with the new generator implementation every rng
is a isolated instance, making it impossible to set a global seed and control all random states (or it's just me unaware of that).
…y:297: DeprecationWarning: Use FrechetCellFilter for better convergence w.r.t. cell variables." This reverts commit b60f5e4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @DanielYang59! 👍
No problem, thanks for merging this. |
Summary
test_wandb_log_frequency
use temporary path to avoid creating files locallyRelocate to another PR
NPY201
rule run seems to pass (https://numpy.org/doc/stable/numpy_2_0_migration_guide.html):