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 #949

Merged
merged 8 commits into from
Oct 6, 2024
Merged

Support NumPy 2 #949

merged 8 commits into from
Oct 6, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Oct 6, 2024

Summary

  • Support NumPy 2
  • Test both NumPy 1 and NumPy 2
  • pytest workflow only show top 20 duration instead of all, otherwise the log page is too long to navigate through

Future PR

  • Completely migrate to pyproject.toml

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Oct 6, 2024

@ml-evs It seems unit test are passing for both numpy 1 and numpy 2, is there any reason to pin numpy < 2 that I missed?

The test failure in auto-gen-release should be owing to restricted access to repo secrets from a fork:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

Type:     clojure.lang.ExceptionInfo
Message:  Expected environment variable to be set: GITHUB_TOKEN
Data:     {:env/name "GITHUB_TOKEN"}
Location: /var/src/release-on-push-action/src/release_on_push_action/core.clj:13:7

Copy link
Collaborator

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ml-evs It seems unit test are passing for both numpy 1 and numpy 2, is there any reason to pin numpy < 2 that I missed?

Not that I know of, though can imagine that test coverage isn't good enough to say for sure. I'm happy to merge this and see if anything breaks downstream

pyproject.toml Outdated Show resolved Hide resolved
@DanielYang59
Copy link
Contributor Author

Not that I know of, though can imagine that test coverage isn't good enough to say for sure.

Thanks for your reply, in pymatgen we have seen a lot of numpy2 compatible issues surrounding cython code for Windows as the default int type changed for Windows in Numpy 2, but it seem there's no cython code for matminer?

@DanielYang59 DanielYang59 marked this pull request as ready for review October 6, 2024 11:53
@DanielYang59 DanielYang59 marked this pull request as draft October 6, 2024 11:57
@DanielYang59 DanielYang59 marked this pull request as ready for review October 6, 2024 12:01
Copy link
Collaborator

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reply, in pymatgen we have seen a lot of numpy2 compatible issues surrounding cython code for Windows as the default int type changed for Windows in Numpy 2, but it seem there's no cython code for matminer?

Not that I'm aware of, so let's just remove it (as you have done).

Thanks for this!

@ml-evs ml-evs merged commit cc3573b into hackingmaterials:main Oct 6, 2024
3 of 4 checks passed
@DanielYang59 DanielYang59 deleted the numpy2 branch October 6, 2024 12:35
@DanielYang59
Copy link
Contributor Author

no problem, thanks for reviewing. I might push another PR to completely migrate setup.py to pyproject.toml, is there any reason not to migrate at this moment?

@ml-evs
Copy link
Collaborator

ml-evs commented Oct 6, 2024

I might push another PR to completely migrate setup.py to pyproject.toml, is there any reason not to migrate at this moment?

Only lack of time/interest -- I don't mind maintaining pyproject.toml over setup.py long term but I don't think we have enough people to support many other changes

@DanielYang59
Copy link
Contributor Author

Cool, I will give a hand on this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants