BLD: Boost.Math standalone submodule #17432
Conversation
mckib2
requested review from
person142,
rgommers and
larsoner
as code owners
|
Thanks @mckib2. Looks like a good idea, I'll have a closer look once CI is happier. |
|
The failure appears to be an off-by-3 error in the calculation of the kurtosis of the noncentral t distribution, which is likely related to boostorg/math#800. I haven't dug deep enough to know why this fails in only a few of the jobs. |
|
@rgommers CI is happier now. Could you take a look? Update: it's not. I don't think this fixes any of gh-14901 as-is. |
This reverts commit e6e9ca8.
rgommers
added
enhancement
|
I'd prefer to merge this right after rather than <24 hrs before creation of a release branch. It looks pretty clean overall; the removal of warning filters does lead to 14 failures on macOS arm64 though, so that has to be looked at before this can go in. Limiting the relevant filters to macOS arm64 only should be fine I'd think. |
|
Boost.Math 1.81.0 Standalone has been published on our page, and should contain your fixes. If there are things missing we can cherry pick them into a 1.81.1. |
|
@mckib2 I think that's a good summary. Again the error should be safe to ignore because it is a floating point environment flag being set, not a true exception. I don't know enough about how cython/pybind11 work, but you could also see if you could clear the flag right after calling the function using |
|
These changes that just landed in Xcode 14.3 beta may be relevant:
FP strict mode is particularly useful in vectorized code where operations on partially filled vectors can raise FP exceptions. Related, |
|
This code LGTM, ready to go. The one thing I see left to do is figure out how to tell everyone to deal with the moved git submodule. This can be a major pain, when git submodules move and history has zero overlap, weird things happen depending on git version. On Linux with git 2.39.0 I could convince it to work with a few |
Huh, I have not had trouble with this using I tested this tonight - the last time I had worked on this branch locally was in early February, and the commit we're at now (9a864d3c520c5fa89043ee7c3edd5ba65fea8cbc) wasn't even available then. Does this sound like it might work for others @rgommers? If so, I suppose the question then is whether we want to create a scipy fork of boost math instead of fetching from |
Exactly - it requires talking to Nicholas and then a bunch of commands. Which means that if we merge this, every contributor who pulls the change into
This either has to be well-tested so we're sure it works, and then communicated. Or, better, why not add a new submodule and removing the old one instead of overwriting an existing one with a new upstream? I think that if you'd add
That wasn't my point and won't make any difference. So I don't think it's needed. |
|
Adding this as a new submodule instead does sound easier then. I just mentioned the repo fork because we use forks for other submodules. I wasn't sure whether we do that as a matter of form or only when there is a particular need. |
We did this for a couple of reasons: ensuring that the repo is stable, because a repo delete or history rewrite action would break our checkouts; and the ability to apply patches, which we can't do directly on upstream. However, for Boost.Math I think we can trust it to be stable, and patch-wise it looks like things will go more smoothly to do that directly upstream, since the project is quite well maintained. So no need for a fork here. |
|
Got it. That was my guess. @mckib2 could you make this a new repo so we can see if the submodule update is easier? |
|
Removed |
|
@rgommers Nicholas created a new |
|
This builds without issue on my machine after running |
|
Builds for me on Ubuntu 22.04 using Python 3.11 and Meson. Also, setup.py works with the same setup. I do get a lot of untracked files with the We can add that to |
|
Thanks for testing this @tirthasheshpatel @steppi! @rgommers when this merges, I could send to the mailing list:
|
|
That sounds great to me, thanks! Ready to merge it sounds like, go ahead I'd say. |
|
As info resulted in I ended up reverting to some old commit on main, removing and adding upstream, and then it worked again. |
|
Huh. Good to know. When you first posted this, you wrote that |
That was a false positive, I wrote it too fast and then deleted this part to limit confusion of possible readers. Sorry for that. |
Reference issue
What does this implement/fix?
Additional information