Scipy: Fix recursive functions #4822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Non recursive functions declare all their locals as static, ones marked "recursive" need them to be proper local variables. Not sure if non recursive funcs need static locals, let's try making all locals non-static first.
|
Looks like I get Pyodide fatal errors on this PR, for example: End of the output: |
|
The problem is that if you say |
This was referenced Jun 5, 2024
|
Still the same issue as #4822 (comment) in the last iteration of the PR. Quicky looking it seems like Here is a snippet that reproduces the issue in this PR (simplified from import numpy as np
from numpy import cos, sin, pi
from numpy.testing import (assert_equal, assert_almost_equal, assert_allclose,
assert_, suppress_warnings)
from scipy.integrate import qmc_quad
from scipy import stats, special
def func(x):
return stats.multivariate_normal.pdf(x.T, mean, cov)
n_points=2**8
n_estimates=8
signs=np.ones(2)
ndim = 2
mean = np.zeros(ndim)
cov = np.eye(ndim)
rng = np.random.default_rng(2879434385674690281)
qrng = stats.qmc.Sobol(ndim, seed=rng)
a = np.zeros(ndim)
b = np.ones(ndim) * signs
print('before qmc_quad', flush=True)
res = qmc_quad(func, a, b, n_points=n_points,
n_estimates=n_estimates, qrng=qrng)
print('after qmc_quad', flush=True)
ref = stats.multivariate_normal.cdf(b, mean, cov, lower_limit=a)
print('before stdtdrit', flush=True)
atol = special.stdtrit(n_estimates-1, 0.995) * res.standard_error # 99% CI
print('after stdtdrit', flush=True)
assert_allclose(res.integral, ref, atol=atol)
assert np.prod(signs)*res.integral > 0
rng = np.random.default_rng(2879434385674690281)
qrng = stats.qmc.Sobol(ndim, seed=rng)
print('before qmc_quad', flush=True)
logres = qmc_quad(lambda *args: np.log(func(*args)), a, b,
n_points=n_points, n_estimates=n_estimates,
log=True, qrng=qrng)
print('after qmc_quad', flush=True)
assert_allclose(np.exp(logres.integral), res.integral, rtol=1e-14)
assert np.imag(logres.integral) == (np.pi if np.prod(signs) < 0 else 0)
assert_allclose(np.exp(logres.standard_error),
res.standard_error, rtol=1e-14, atol=1e-16)I get this kind of info |
|
Thanks for the reproduction @lesteve. |
|
Okay I think I got it. |
|
The dblquad test does not seem to pass with your latest fixes 😅 ... For example, node build log: There is this warning as well just before that is likely relevant: |
|
Hmm still doesn't work. It's just a matter of futzing with it at this point I think, some of the commits make the dblquad test pass so I definitely have the right approach. |
ryanking13
reviewed
Jun 25, 2024
There was a problem hiding this comment.
Thanks for handling fortran issues @hoodmane! Let me port the f2c fixes to pyodide/pyodide-build.
|
FYI looks like this fixed quite a number of issues in the Scipy test suite, see lesteve/scipy-tests-pyodide#37. In particular:
|
ryanking13
added a commit
to pyodide/pyodide-build
that referenced
this pull request
Jun 28, 2024
Ports pyodide/pyodide#4822 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
hoodmane
added a commit
to hoodmane/pyodide
that referenced
this pull request
Jul 24, 2024
Non recursive functions declare all their locals as static, ones marked "recursive" need them to be proper local variables. The Fortran files each contain exactly one function. If the one function is recursive, we label the source file with a comment. Then when processing the C file, if it contains the recursive marker comment, we remove the `static` modifiers from the variables. At this point, we really ought to consider modifying f2c itself...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Non recursive functions declare all their locals as static, ones marked "recursive" need them to be proper local variables. Not sure if non recursive funcs need static locals, let's try making all locals non-static first. If this doesn't work, we can locate functions marked recursive and only modify them.
Fixes #4818.
@lesteve can you check the status of the scipy tests here?