-
Notifications
You must be signed in to change notification settings - Fork 2
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
[refactor] Build packages with cibuildwheels
#55
[refactor] Build packages with cibuildwheels
#55
Conversation
That way, we do not need to explicitly state which versions of Python we build for. It also simplifies some of the configuration It seems we need to explicitly include pybind11 directories for some builds
There are no pre-compiled wheels of scipy available for PyPy, so we run the tests with our own implementation of chi2 in pure Python
macOS 11 runners are no longer supported. Intel macs on GitHub Actions is now macOS 13.
It is only used in the build step Nor do we need to re-upload the files if cache was hit
bc3c6d6
to
8deb937
Compare
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.
This looked good! I had only a few notes, and you can consider doing something about them at your own discretion.
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.
This is a good decision. If Scipy is used for only one test and it's hard to compile - this really is the right thing to do.
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.
I have no comments on the implementation itself, I don't know anything about the distribution. I assume you copied it from somewhere, but kudos if you made it yourself.
If you copied it, I would prefer a source (just in case we later want to debug or something).
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.
The implementation is pretty mus straight from the definition of the probability density function (as seen on Wikipedia).
The "percentage point function" was essentially made from some experimentation (to find where the pdf was 0.0
and from the definition of such a function.
The doc-strings where copied from scipy
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.
If it turns out my implementation is correct, then we may want to remove scipy
from the tests, as you suggested. It will probably make them somewhat faster, as it takes a little time to download and install scipy.
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.
It looks ok. I have checked the chi2 function and the inverse cumulative, but I have not much to contribute with regarding the yml file and cmake.
Replace the custom GitHub Actions steps for building the wheels with
cibuildwheels
, which will build the wheel for all supported versions of Python.This was originally part of #36, but I created a separate PR to make it easier to see the relevant changes that where necessary for Windows support.
There are some minor changes in this PR to accommodate PyPY which we did not support before.
Since
cibuildwheel
support it out of the box and it seems to work fine, we might as well start to publish wheels for it.