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

Bumped min numpy version to 1.18 #324

Merged
merged 12 commits into from
Aug 6, 2024
Merged

Conversation

manodeep
Copy link
Owner

No description provided.

manodeep and others added 8 commits July 29, 2024 17:24
Now minimum supported numpy is 1.18 and minimum supported python (py3) is 3.9
Probably pissed off some software gremlins by allowing that combo!
* Fix headers and include paths for NumPy 2 (currently breaks NumPy 1 build support)

* Fix NumPy 1 build by using numpy.get_include()

---------

Co-authored-by: Lehman Garrison <lgarrison@flatironinstitute.org>
Too much hassle trying to maintain py2
@pep8speaks
Copy link

pep8speaks commented Aug 1, 2024

Hello @manodeep! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-08-01 04:27:26 UTC

@manodeep
Copy link
Owner Author

manodeep commented Aug 1, 2024

@lgarrison Will you please take a look that I didn't reduce the combinations of os/compiler/python/numpy too much? (really, we need to have a separate "does-this-even-compile-and-run-a-simple-thing" workflow and only run the tests on a smaller subset of hardware)

I have dropped python2 completely - so the next release should be a major release (and not a minor/patch release) - possibly even Corrfunc3.0.0. What do you think? Also, should we also take out the code for python2 within the CPython extensions?

@manodeep manodeep requested a review from lgarrison August 1, 2024 04:32
@manodeep
Copy link
Owner Author

manodeep commented Aug 1, 2024

Hmm - now that I think about it a bit more - may be I should drop py2 and numpy in CI first and release a patch version first (ie revert the change to setup.py)

Copy link
Collaborator

@lgarrison lgarrison left a comment

Choose a reason for hiding this comment

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

Either way is fine with me! I think this looks like good test coverage. Happy to have Corrfunc 3 as the next release, that makes it easy for people to find a version that works with NumPy 2.

@manodeep
Copy link
Owner Author

manodeep commented Aug 6, 2024

@lgarrison Looks like there were plans to rename the package to corrfunc rather than Corrfunc for Corrfunc3.0. That also seems appropriate - what do you think?

@manodeep
Copy link
Owner Author

manodeep commented Aug 6, 2024

I reverted the drop of py2 to create one final patch release with python2. Once the tests pass, I am going to merge the branch in and release 2.5.3.

I will then create a new branch to start the process for releasing Corrfunc3

@manodeep manodeep merged commit 77dc72e into master Aug 6, 2024
@manodeep manodeep deleted the manodeep-update-numpy-and-ci branch August 6, 2024 04:33
@lgarrison
Copy link
Collaborator

@lgarrison Looks like there were plans to rename the package to corrfunc rather than Corrfunc for Corrfunc3.0. That also seems appropriate - what do you think?

Hmm, this would be a breaking change for a lot of people. Should we have a deprecation period, or make the change but at least emit an informative error when using the old name?

@manodeep
Copy link
Owner Author

manodeep commented Aug 7, 2024

How would we do a deprecation period? The biggest motivation to change the package name is to conform to the python convention - which might be required if we want to migrate to newer packaging methods (i.e., migrate out of setuptools).

If there is an import error, then the error handling would be done by python - I don't think we would be able to emit any message at all - right? Or could we add a dummy Corrfunc.py file within the new package and use that somehow to emit the info message?

@lgarrison
Copy link
Collaborator

Yeah, I was thinking of something like a warning period where import Corrfunc would work but emit a message telling people to switch to import corrfunc. I don't know the best way to do this off the top of my head; maybe a symlink of Corrfunc/ to corrfunc/ (with logic to check which path was used in the import) or maybe a Corrfunc.py.

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.

3 participants