Skip to content
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

Support NumPy 2 #202

Merged
merged 18 commits into from
Sep 12, 2024
Merged

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Sep 12, 2024

Summary

Support NumPy 2, previously #184

@janosh
Copy link
Collaborator

janosh commented Sep 12, 2024

incidentally, one way to get around the need to manually approve CI runs on every commit would be to merge a small PR instantly. if you want you could fix a few of the DOC errors

ruff check . --select DOC --preview

and submit a PR for that which we merge right away

@DanielYang59 DanielYang59 changed the title Migrate to NumPy 2 Support NumPy 2 Sep 12, 2024
@DanielYang59

This comment was marked as resolved.

@janosh
Copy link
Collaborator

janosh commented Sep 12, 2024

why are only the windows tests failing? is the value of dataset.magmoms platform dependent?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Sep 12, 2024

You really had me here :) I haven't looked into the source code yet (thought that's a known issue), would have a look tomorrow.

Looks like dataset.magmoms is missing for windows for some reason:

assert len(dataset_dict["magmom"]) == len(dataset_dict["force"])
E       assert 0 == 1

@DanielYang59
Copy link
Contributor Author

Again:

for line in reverse_readfile(outcar_path):
clean = line.strip()
all_lines.append(clean)

@janosh
Copy link
Collaborator

janosh commented Sep 12, 2024

reverse_readfile

your current arch nemesis strikes again 🤣

@DanielYang59
Copy link
Contributor Author

That's sad, looks like we cannot merge this until reverse readline is fixed, because this wouldn't be a working version for windows users.

your current arch nemesis strikes again 🤣

OK! my time off is over, going back to work tomorrow :)

@DanielYang59
Copy link
Contributor Author

Any comment on the following message (seemingly something terrible)?

tests\test_encoders.py .................
!!!!!! the two directed edges are equal but this operation is not supposed to happen
!!!!!! the two directed edges are equal but this operation is not supposed to happen
tests\test_graph.py ........

@janosh
Copy link
Collaborator

janosh commented Sep 12, 2024

Any comment on the following message (seemingly something terrible)?

i think those messages are expected (maybe from a test meant to check that error message)

@janosh
Copy link
Collaborator

janosh commented Sep 12, 2024

i'm fine with merging this and having a main branch where the parse_vasp_dir has issues on windows. we can wait with a package release until those are resolved

@bowen-bd
Copy link
Collaborator

Thanks a lot for the migration to np 2! @DanielYang59 @janosh
I just fixed the message test_graph.py and will merge this PR now

@bowen-bd bowen-bd marked this pull request as ready for review September 12, 2024 23:47
@bowen-bd bowen-bd merged commit 035abf2 into CederGroupHub:main Sep 12, 2024
9 checks passed
@janosh janosh added pkg Package compatibility Compatibility with different OS, Python, PyTorch, numpy, etc. labels Sep 12, 2024
@DanielYang59 DanielYang59 deleted the really-migrate-to-np2 branch September 13, 2024 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with different OS, Python, PyTorch, numpy, etc. pkg Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants